Skip to content

Commit

Permalink
refactor-request-queue: fixes
Browse files Browse the repository at this point in the history
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.
dpatti committed Apr 26, 2021
1 parent e6fcd1b commit 77f216b
Showing 1 changed file with 6 additions and 9 deletions.
15 changes: 6 additions & 9 deletions lib/server_connection.ml
Original file line number Diff line number Diff line change
@@ -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;

0 comments on commit 77f216b

Please sign in to comment.