From cad867785da525d0637511b427d845dd6737cf5e Mon Sep 17 00:00:00 2001 From: David House Date: Thu, 29 Apr 2021 19:42:09 +0100 Subject: [PATCH] Don't hang the reader thread after write eof In many runtimes, there are separate reader and writer threads that drive the reading and writing from httpaf independently. So a thing that can happen is: - A request arrives. - The response handler begins but does not finish responding to it. - The writer thread calls [Server_connection.report_write_result `Closed]. - The reader thread delivers another request. In this case, httpaf will never deliver the second request to the handler, because the first request's output_state never gets to Complete. We have no hope of responding to the second request, but we should still deliver it to the handler in case the request has side-effects (e.g. as many POST requests do). This PR fixes this by noticing when the writer is closed in [Reqd.output_state] and just always returning [Complete] in this case, as no more output progress can be made. --- lib/reqd.ml | 9 +++++++-- lib_test/test_server_connection.ml | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/reqd.ml b/lib/reqd.ml index 8277a637..2e9d4b8a 100644 --- a/lib/reqd.ml +++ b/lib/reqd.ml @@ -235,12 +235,17 @@ let output_state t : Output_state.t = match t.response_state with | Fixed _ -> Complete | Streaming (_, response_body) -> - if Body.has_pending_output response_body + if Writer.is_closed t.writer + then Complete + else if Body.has_pending_output response_body then Ready else if Body.is_closed response_body then Complete else Waiting - | Waiting -> Waiting + | Waiting -> + if Writer.is_closed t.writer + then Complete + else Waiting ;; let flush_request_body t = diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index e1b0e696..6783db65 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -894,6 +894,29 @@ let test_response_finished_before_body_read () = write_response t response ~body:"done"; ;; +let test_can_read_more_requests_after_write_eof () = + let request = Request.create `GET "/" ~headers:(Headers.encoding_fixed 0) in + let reqd = ref None in + let request_handler reqd' = reqd := Some reqd' in + let t = create request_handler in + let response = Response.create `OK ~headers:Headers.encoding_chunked in + read_request t request; + Reqd.respond_with_streaming (Option.get !reqd) response ~flush_headers_immediately:true + |> (ignore : [ `write ] Body.t -> unit); + write_eof t; + (* In many runtimes, there are separate reader and writer threads that drive the reading + and writing from httpaf independently. So just because the writer thread has told us + that the socket is closed doesn't mean we won't get a bunch more requests delivered + to us from the reader thread. We should be ready to receive them, and call the + request handler for them, even if there is no possibility of writing responses (e.g. + those requests might be side-effecting requests like POST requests). *) + reqd := None; + reader_ready t; + read_request t request; + Alcotest.(check bool) "request handler fired" true (Option.is_some !reqd) +;; + + let tests = [ "initial reader state" , `Quick, test_initial_reader_state ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof @@ -922,4 +945,5 @@ let tests = ; "multiple requests with connection close", `Quick, test_multiple_requests_in_single_read_with_close ; "parse failure after checkpoint", `Quick, test_parse_failure_after_checkpoint ; "response finished before body read", `Quick, test_response_finished_before_body_read + ; "can read more requests after write eof", `Quick, test_can_read_more_requests_after_write_eof ]