From 641edb7d0381dd7c82afde05afd83e89afefd8a6 Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sat, 22 May 2021 15:18:53 -0400 Subject: [PATCH 1/2] fix read loop with `shutdown` When you shutdown the connection and we close the reader, a surprising thing happens under certain conditions: the read loop wakes up, asks for the next read operation, and is told to yield. It tries to yield, and then the state machine raises -- you shouldn't be yielding if the connection is closed! The reason for this is our explicit return of [`Yield] in `_final_read_operation_for`. As mentioned in my lengthy comment, I think the real issue here is that if we want to circumvent `Reader.next`, we have to make sure we're not yielding when the reader cannot take any more input anyway. I thought of fixing this in a few ways, like adding explicit yielding and continuing mechanisms to the reader at request boundaries, which is something that has come up in the past as being a bit tricky to reason about. I also considered just tossing this check at the top of `next_read_operation`, but it feels like the kind of thing that I'd later ask "why is this necessary?" I want so badly for the reading and writing behaviors to be parallel, so I did something that is meant to feel less permanent. This test also fails before the `refactor-request-queue` branch. I found it curious, but then realized that `Server_connection.shutdown` is not used in either of our runtimes, so it's not too surprising that we didn't have decent coverage of it. Note on testing --------------- One of the solutions I tried was to change the `peristent_connection` branch to also check closed state. But then I realized that you could have a closed reader but still want to process more connections, so you wouldn't want to simply shut the whole thing down. I added a missing test that will fail if you do the wrong thing, now. --- lib/server_connection.ml | 14 +++++++- lib_test/test_server_connection.ml | 55 ++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/lib/server_connection.ml b/lib/server_connection.ml index f81cd5bd..c4b3680e 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -207,7 +207,19 @@ and _final_read_operation_for t reqd = Reader.next t.reader; ) else ( match Reqd.output_state reqd with - | Waiting | Ready -> `Yield + | Waiting | Ready -> + (* XXX(dpatti): This is a way in which the reader and writer are not + parallel -- we tell the writer when it needs to yield but the reader is + always asking for more data. This is the only branch in either + operation function that does not return `(Reader|Writer).next`, which + means there are surprising states you can get into. For example, we ask + the runtime to yield but then raise when it tries to because the reader + is closed. I don't think checking `is_closed` here makes sense + semantically, but I don't think checking it in `_next_read_operation` + makes sense either. I chose here so I could describe why. *) + if Reader.is_closed t.reader + then Reader.next t.reader + else `Yield | Complete -> advance_request_queue t; _next_read_operation t; diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index e1b0e696..752d994c 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -40,6 +40,8 @@ module Runtime : sig val on_writer_unyield : t -> (unit -> unit) -> bool ref val report_exn : t -> exn -> unit + + val shutdown : t -> unit end = struct open Server_connection @@ -158,32 +160,35 @@ end = struct ;; let report_exn t = Server_connection.report_exn t.server_connection + + let shutdown t = Server_connection.shutdown t.server_connection end open Runtime -let read t str ~off ~len = - do_read t (fun conn -> Server_connection.read conn str ~off ~len) +let read ?(eof=false) t str ~off ~len = + do_read t (fun conn -> + if eof + then Server_connection.read_eof conn str ~off ~len + else Server_connection.read conn str ~off ~len) ;; -let read_eof t str ~off ~len = - do_read t (fun conn -> Server_connection.read_eof conn str ~off ~len) -;; +let read_eof = read ~eof:true -let feed_string t str = +let feed_string ?eof t str = let len = String.length str in let input = Bigstringaf.of_string str ~off:0 ~len in - read t input ~off:0 ~len + read ?eof t input ~off:0 ~len ;; -let read_string t str = - let c = feed_string t str in +let read_string ?eof t str = + let c = feed_string ?eof t str in Alcotest.(check int) "read consumes all input" (String.length str) c; ;; -let read_request t r = +let read_request ?eof t r = let request_string = request_to_string r in - read_string t request_string + read_string ?eof t request_string ;; let reader_ready t = @@ -850,6 +855,21 @@ let test_multiple_requests_in_single_read_with_close () = connection_is_shutdown t; ;; +let test_multiple_requests_in_single_read_with_eof () = + let response = Response.create `OK in + let t = + create (fun reqd -> Reqd.respond_with_string reqd response "") + in + let reqs = + request_to_string (Request.create `GET "/") ^ + request_to_string (Request.create `GET "/") + in + read_string t reqs ~eof:true; + write_response t response; + write_response t response; + connection_is_shutdown t; +;; + let test_parse_failure_after_checkpoint () = let error_queue = ref None in let error_handler ?request:_ error _start_response = @@ -894,6 +914,17 @@ let test_response_finished_before_body_read () = write_response t response ~body:"done"; ;; +let test_shutdown_in_request_handler () = + let request = Request.create `GET "/" in + let rec t = + lazy (create (fun _ -> shutdown (Lazy.force t))) + in + let t = Lazy.force t in + read_request t request; + reader_closed t; + writer_closed t +;; + let tests = [ "initial reader state" , `Quick, test_initial_reader_state ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof @@ -920,6 +951,8 @@ let tests = ; "multiple requests in single read", `Quick, test_multiple_requests_in_single_read ; "multiple async requests in single read", `Quick, test_multiple_async_requests_in_single_read ; "multiple requests with connection close", `Quick, test_multiple_requests_in_single_read_with_close + ; "multiple requests with eof", `Quick, test_multiple_requests_in_single_read_with_eof ; "parse failure after checkpoint", `Quick, test_parse_failure_after_checkpoint ; "response finished before body read", `Quick, test_response_finished_before_body_read + ; "shutdown in request handler", `Quick, test_shutdown_in_request_handler ] From c9e7a0777957514012cdb045ee543dd4434a84d8 Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sat, 22 May 2021 16:45:08 -0400 Subject: [PATCH 2/2] add test for shutdown before handler --- lib_test/test_server_connection.ml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 752d994c..056f85dd 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -925,6 +925,24 @@ let test_shutdown_in_request_handler () = writer_closed t ;; +let test_shutdown_during_asynchronous_request () = + let request = Request.create `GET "/" in + let response = Response.create `OK in + let continue = ref (fun () -> ()) in + let t = create (fun reqd -> + continue := (fun () -> + Reqd.respond_with_string reqd response "")) + in + read_request t request; + shutdown t; + (* This is raised from Faraday *) + Alcotest.check_raises "[continue] raises because writer is closed" + (Failure "cannot write to closed writer") + !continue; + reader_closed t; + writer_closed t +;; + let tests = [ "initial reader state" , `Quick, test_initial_reader_state ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof @@ -955,4 +973,5 @@ let tests = ; "parse failure after checkpoint", `Quick, test_parse_failure_after_checkpoint ; "response finished before body read", `Quick, test_response_finished_before_body_read ; "shutdown in request handler", `Quick, test_shutdown_in_request_handler + ; "shutdown during asynchronous request", `Quick, test_shutdown_during_asynchronous_request ]