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.
  • Loading branch information
dpatti committed Apr 2, 2021
1 parent 283c992 commit 568a2d8
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 10 deletions.
15 changes: 6 additions & 9 deletions lib/server_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 ->
Expand All @@ -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;
Expand Down
20 changes: 19 additions & 1 deletion lib_test/test_server_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
;;

Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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
]

0 comments on commit 568a2d8

Please sign in to comment.