From 3178faacbc0be1c555b5f579bb7c1f9814480a40 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 ++++++--------- 1 file changed, 6 insertions(+), 9 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;