From 775b94ffc3b56e58c6791ca9704666605c1ca461 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Sun, 15 Dec 2019 13:48:03 -0800 Subject: [PATCH 1/3] Fix reader getting stuck (never waking up) when using `~flush_headers_immediately:true` in combination with an empty response body --- CHANGES.md | 5 +++++ lib/body.ml | 17 ++++++++++++++--- lib_test/test_httpaf.ml | 42 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e7f88aa..ac8f40e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -48,6 +48,11 @@ Unreleased 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 -------------- diff --git a/lib/body.ml b/lib/body.ml index df911f2..bcb0848 100644 --- a/lib/body.ml +++ b/lib/body.ml @@ -157,6 +157,20 @@ let when_ready_to_write t callback = let transfer_to_writer_with_encoding t ~encoding writer = let faraday = t.faraday in + 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 -> () | `Close -> @@ -178,9 +192,6 @@ let transfer_to_writer_with_encoding t ~encoding writer = buffered := !buffered + lengthv; begin match encoding with | `Fixed _ | `Close_delimited -> - (* Play nicely with [has_pending_output] in the case of a fixed or - close-delimited encoding. *) - t.write_final_if_chunked <- false; Serialize.Writer.schedule_fixed writer iovecs | `Chunked -> Serialize.Writer.schedule_chunk writer iovecs diff --git a/lib_test/test_httpaf.ml b/lib_test/test_httpaf.ml index 63a8982..53f8d5d 100644 --- a/lib_test/test_httpaf.ml +++ b/lib_test/test_httpaf.ml @@ -836,6 +836,46 @@ Accept-Language: en-US,en;q=0.5\r\n\r\n"; writer_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 @@ -859,6 +899,8 @@ Accept-Language: en-US,en;q=0.5\r\n\r\n"; ; "respond with upgrade", `Quick, test_respond_with_upgrade ; "writer unexpected eof", `Quick, test_unexpected_eof ; "input shrunk", `Quick, test_input_shrunk + ; "`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 ] end From 3fa8254a34b7c2807a6e1ef431b786683164a08f Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Sun, 15 Dec 2019 14:02:08 -0800 Subject: [PATCH 2/3] [ci skip] tweak order in changelog to match chronological --- CHANGES.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ac8f40e..0d478c8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,16 @@ Unreleased -------------- +- 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-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-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)) @@ -17,16 +27,6 @@ Unreleased 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-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 From 6e3fd173b378b1041b1dc64f969ec8092213b785 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Sun, 15 Dec 2019 14:06:05 -0800 Subject: [PATCH 3/3] tweak changelog order to match order in which PRs were merged --- CHANGES.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 0d478c8..68c1b35 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,18 +8,6 @@ Unreleased - 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-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-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)) @@ -27,8 +15,20 @@ Unreleased 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, 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