Skip to content

Commit

Permalink
Don't hang the reader thread after write eof
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dhouse-js committed Apr 29, 2021
1 parent a49ea3c commit cad8677
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 2 deletions.
9 changes: 7 additions & 2 deletions lib/reqd.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
24 changes: 24 additions & 0 deletions lib_test/test_server_connection.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
]

0 comments on commit cad8677

Please sign in to comment.