From 568a2d8f496ce87209f21529a379180d46acd96f Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Fri, 2 Apr 2021 15:58:19 -0400 Subject: [PATCH] refactor-request-queue: fixes We basically never want to call `Queue.clear` because the head of the queue has special semantic meaning. Instead, we never try to clear the queue and rely on the fact that the queue will never be advanced. This is easy to reason about because the only time we advance the request queue is when the current request is not persistent. I added an explicit test of this situation to build confidence. Additionally, there was an incorrect assertion that you couldn't finish a write with reads still pending. A test was added upstream and it no longer fails with this fix. The final change was some subtle but unused code. In the write loop, we have something that decides to shutdown the connection if the reader is closed, parallel to the next read operation. But this felt weird, the reader should always be awake in the case that it is closed, which means that either 1) it will shutdown the connection or 2) it will wait for the writer, which will wake the reader once it's advanced the request queue, and then it will shutdown the connection. --- lib/server_connection.ml | 15 ++++++--------- lib_test/test_server_connection.ml | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/lib/server_connection.ml b/lib/server_connection.ml index db6f08ef..f81cd5bd 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -155,7 +155,6 @@ let error_code t = else None let shutdown t = - Queue.clear t.request_queue; shutdown_reader t; shutdown_writer t; wakeup_reader t; @@ -190,7 +189,8 @@ let advance_request_queue t = ;; let rec _next_read_operation t = - if not (is_active t) then ( + if not (is_active t) + then ( if Reader.is_closed t.reader then shutdown t; Reader.next t.reader @@ -216,7 +216,6 @@ and _final_read_operation_for t reqd = let next_read_operation t = match _next_read_operation t with - (* XXX(dpatti): These two [`Error _] constructors are never returned *) | `Error (`Parse _) -> set_error_and_handle t `Bad_request; `Close | `Error (`Bad_request request) -> set_error_and_handle ~request t `Bad_request; `Close | (`Read | `Yield | `Close) as operation -> operation @@ -248,11 +247,9 @@ let read_eof t bs ~off ~len = read_with_more t bs ~off ~len Complete let rec _next_write_operation t = - if not (is_active t) then ( - if Reader.is_closed t.reader - then shutdown t; - Writer.next t.writer - ) else ( + if not (is_active t) + then Writer.next t.writer + else ( let reqd = current_reqd_exn t in match Reqd.output_state reqd with | Waiting -> @@ -274,7 +271,7 @@ and _final_write_operation_for t reqd = Writer.next t.writer; ) else ( match Reqd.input_state reqd with - | Ready -> assert false + | Ready -> Writer.next t.writer; | Complete -> advance_request_queue t; _next_write_operation t; diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index a7352911..a836dd79 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -647,7 +647,7 @@ let test_multiple_requests_in_single_read () = request_to_string (Request.create `GET "/") in read_string t reqs; - reader_yielded t; + reader_ready t; Alcotest.(check int) "fired handler of both requests" 2 !reqs_handled ;; @@ -677,6 +677,23 @@ let test_multiple_async_requests_in_single_read () = reader_ready t; ;; +let test_multiple_requests_in_single_read_with_close () = + let reqs_handled = ref 0 in + let t = + create (fun reqd -> + reqs_handled := !reqs_handled + 1; + let response = Response.create `OK ~headers:Headers.connection_close in + 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; + reader_closed t; + Alcotest.(check int) "fired handler of first requests" 1 !reqs_handled +;; + let test_parse_failure_after_checkpoint () = let error_queue = ref None in let error_handler ?request:_ error _start_response = @@ -737,6 +754,7 @@ let tests = ; "bad request", `Quick, test_bad_request ; "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 ; "parse failure after checkpoint", `Quick, test_parse_failure_after_checkpoint ; "response finished before body read", `Quick, test_response_finished_before_body_read ]