Fix reader getting stuck (never waking up) when using `~flush_headers…

…_immediately:true` in combination with an empty response body (#34)

CHANGES.md

@@ -1,34 +1,34 @@
 Unreleased
 --------------
 
-- httpaf-lwt-unix: replace the `dune` file (previously written in OCaml) with a
-  `(select)` form to avoid depending on `ocamlfind`
-  ([#18](https://github.com/anmonteiro/httpaf/pull/18))
-- httpaf-mirage: depend on `mirage-conduit` instead of `conduit-mirage`,
-  effectively placing a lower bound of OCaml 4.07 on httpaf-mirage
-  ([#16](https://github.com/anmonteiro/httpaf/pull/16))
-- httpaf-lwt, httpaf-lwt-unix, httpaf-mirage: deduplicate interface code via a
-  common `Httpaf_lwt_intf` interface
-  ([#13](https://github.com/anmonteiro/httpaf/pull/13))
-- httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-async, httpaf-mirage: add support
-  for persistent connections and pipelining in the client implementations
-  ([#5](https://github.com/anmonteiro/httpaf/pull/5))
-- httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-async, httpaf-mirage: add support
-  for switching protocols, e.g. to a WebSocket connection, via a new function
-  `Reqd.respond_with_upgrade`
-  ([#8](https://github.com/anmonteiro/httpaf/pull/8))
 - httpaf-lwt-unix: add support for HTTPS in the Lwt runtime, either via OpenSSL
   or `ocaml-tls` ([#2](https://github.com/anmonteiro/httpaf/pull/2))
 - httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-mirage: Add a Mirage adapter
   ([#3](https://github.com/anmonteiro/httpaf/pull/3))
 - Add an `esy.json` file ([#6](https://github.com/anmonteiro/httpaf/pull/6))
 - httpaf: Catch parsing errors and hand them to `error_handler` in
   `Server_connection` ([#4](https://github.com/anmonteiro/httpaf/pull/4))
+- httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-async, httpaf-mirage: add support
+  for persistent connections and pipelining in the client implementations
+  ([#5](https://github.com/anmonteiro/httpaf/pull/5))
+- httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-async, httpaf-mirage: add support
+  for switching protocols, e.g. to a WebSocket connection, via a new function
+  `Reqd.respond_with_upgrade`
+  ([#8](https://github.com/anmonteiro/httpaf/pull/8))
+- httpaf-lwt, httpaf-lwt-unix, httpaf-mirage: deduplicate interface code via a
+  common `Httpaf_lwt_intf` interface
+  ([#13](https://github.com/anmonteiro/httpaf/pull/13))
+- httpaf-mirage: depend on `mirage-conduit` instead of `conduit-mirage`,
+  effectively placing a lower bound of OCaml 4.07 on httpaf-mirage
+  ([#16](https://github.com/anmonteiro/httpaf/pull/16))
+- httpaf-lwt-unix: replace the `dune` file (previously written in OCaml) with a
+  `(select)` form to avoid depending on `ocamlfind`
+  ([#18](https://github.com/anmonteiro/httpaf/pull/18))
+- httpaf: Shutdown the writer after closing a non chunk-encoded request body on
+  the client ([#23](https://github.com/anmonteiro/httpaf/pull/23))
 - httpaf-mirage: Adapt to Mirage 3.7 interfaces. `httpaf_mirage` now requires
   `conduit-mirage` >= 2.0.2 and `mirage-flow` >= 2.0.0
   ([#24](https://github.com/anmonteiro/httpaf/pull/24))
-- httpaf: Shutdown the writer after closing a non chunk-encoded request body on
-  the client ([#23](https://github.com/anmonteiro/httpaf/pull/23))
 - httpaf, httpaf-lwt, httpaf-lwt-unix, httpaf-async, httpaf-mirage: after
   switching protocols, close the connection / file descriptors when the upgrade
   handler's returned promise resolves
@@ -44,6 +44,15 @@ Unreleased
   ([#30](https://github.com/anmonteiro/httpaf/pull/30))
 - httpaf, httpaf-lwt, httpaf-async: Add support for upgrading connections on
   the client. ([#31](https://github.com/anmonteiro/httpaf/pull/31))
+- httpaf: Fix the order of parsed request / response headers to match what it's
+  advertised in the interface file. They are now served to the respective
+  handlers in the original transmission order
+  ([#32](https://github.com/anmonteiro/httpaf/pull/32))
+- httpaf: Fix persistent connections getting stuck (reader never waking up)
+  when using `~flush_headers_immediately:true` in combination with an empty
+  response body ([#34](https://github.com/anmonteiro/httpaf/pull/34)). This is
+  a fix for an issue opened in the upstream repo:
+  [inhabitedtype/httpaf#162](https://github.com/inhabitedtype/httpaf/issues/162)
 
 httpaf (upstream) 0.6.5
 --------------

lib/body.ml

@@ -156,11 +156,19 @@ let when_ready_to_write t callback =
 
 let transfer_to_writer_with_encoding t ~encoding writer =
   let faraday = t.faraday in
-  (* Play nicely with [has_pending_output] in the case of a fixed or
-     close-delimited encoding. *)
-  begin match encoding with
-  | `Fixed _ | `Close_delimited -> t.write_final_if_chunked <- false;
-  | `Chunked -> ()
+  begin match t.write_final_if_chunked, encoding with
+  | true, (`Fixed _ | `Close_delimited) ->
+    (* Play nicely with [has_pending_output] in the case of a fixed or
+       close-delimited encoding.
+
+       Immediately set `t.write_final_if_chunked` to `false` because we may
+       not have another opportunity to do so before advancing the request
+       queue. *)
+    t.write_final_if_chunked <- false;
+  | false, (`Fixed _ | `Close_delimited)
+  | _, `Chunked ->
+    (* Handled explicitly later when closing the writer. *)
+    ()
   end;
   begin match Faraday.operation faraday with
   | `Yield -> ()

lib_test/test_server_connection.ml

@@ -683,6 +683,46 @@ let test_malformed_request_streaming_error_response () =
   Alcotest.(check bool) "Connection is shutdown" true (is_closed t);
 ;;
 
+let test_immediate_flush_empty_body () =
+  let reader_woken_up = ref false in
+  let response = Response.create `OK in
+  let request_handler reqd =
+    let resp_body = Reqd.respond_with_streaming
+      ~flush_headers_immediately:true reqd response
+    in
+    Body.close_writer resp_body;
+  in
+  let t = create ~error_handler request_handler in
+  reader_ready t;
+  yield_writer t (fun () ->
+    write_response t ~body:"" response);
+  read_request t (Request.create `GET "/");
+  yield_reader t (fun () -> reader_woken_up := true);
+  writer_yielded t;
+  Alcotest.(check bool) "Reader woken up"
+    true !reader_woken_up;
+;;
+
+let test_empty_body_no_immediate_flush () =
+  let reader_woken_up = ref false in
+  let response = Response.create `OK in
+  let request_handler reqd =
+    let resp_body = Reqd.respond_with_streaming
+      ~flush_headers_immediately:false reqd response
+    in
+    Body.close_writer resp_body;
+  in
+  let t = create ~error_handler request_handler in
+  reader_ready t;
+  writer_yielded t;
+  read_request t (Request.create `GET "/");
+  yield_reader t (fun () -> reader_woken_up := true);
+  write_response t ~body:"" response;
+  writer_yielded t;
+  Alcotest.(check bool) "Reader woken up"
+    true !reader_woken_up;
+;;
+
 let tests =
   [ "initial reader state"  , `Quick, test_initial_reader_state
   ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof
@@ -708,4 +748,6 @@ let tests =
   ; "multiple malformed requests?", `Quick, test_malformed_request_async_multiple_errors
   ; "malformed request (EOF)", `Quick, test_malformed_request_eof
   ; "malformed request, streaming response", `Quick, test_malformed_request_streaming_error_response
+  ; "`flush_headers_immediately` with empty body", `Quick, test_immediate_flush_empty_body
+  ; "empty body with no immediate flush", `Quick, test_empty_body_no_immediate_flush
   ]