From 14bf7316f7e6eb5f5237c9f0f7012704e4e010cf Mon Sep 17 00:00:00 2001 From: David House Date: Thu, 29 Apr 2021 16:11:53 +0100 Subject: [PATCH 1/4] Server_connection.shutdown should flush the response body before closing the writer. --- lib/server_connection.ml | 1 + lib_test/test_server_connection.ml | 48 ++++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/lib/server_connection.ml b/lib/server_connection.ml index c4b3680e..a34a0ebb 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -144,6 +144,7 @@ let shutdown_reader t = else wakeup_reader t let shutdown_writer t = + if is_active t then Reqd.flush_response_body (current_reqd_exn t); Writer.close t.writer; if is_active t then Reqd.close_request_body (current_reqd_exn t) diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 056f85dd..004a0382 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -53,6 +53,7 @@ end = struct ; write_loop : (unit -> unit) ; mutable read_unyield_hook : (unit -> unit) option ; mutable write_unyield_hook : (unit -> unit) option + ; mutable stopped : bool } let rec read_step t = @@ -63,12 +64,13 @@ end = struct | `Yield -> trace "reader: Yield"; t.read_operation <- `Yield; - yield_reader t.server_connection (fun () -> - trace "reader: Yield callback"; - read_step t; - t.read_unyield_hook |> Option.iter (fun f -> - t.read_unyield_hook <- None; - f ())) + if not t.stopped then + yield_reader t.server_connection (fun () -> + trace "reader: Yield callback"; + read_step t; + t.read_unyield_hook |> Option.iter (fun f -> + t.read_unyield_hook <- None; + f ())) | `Close -> trace "reader: Close"; t.read_operation <- `Close @@ -82,12 +84,13 @@ end = struct | `Yield -> t.write_operation <- `Yield; trace "writer: Yield"; - yield_writer t.server_connection (fun () -> - trace "writer: Yield callback"; - write_step t; - t.write_unyield_hook |> Option.iter (fun f -> - t.write_unyield_hook <- None; - f ())) + if not t.stopped then + yield_writer t.server_connection (fun () -> + trace "writer: Yield callback"; + write_step t; + t.write_unyield_hook |> Option.iter (fun f -> + t.write_unyield_hook <- None; + f ())) | `Close n -> trace "writer: Close"; t.write_operation <- `Close n @@ -103,6 +106,7 @@ end = struct ; write_loop = (fun () -> write_step (Lazy.force_val t)) ; read_unyield_hook = None ; write_unyield_hook = None + ; stopped = false }) in let t = Lazy.force_val t in @@ -161,7 +165,9 @@ end = struct let report_exn t = Server_connection.report_exn t.server_connection - let shutdown t = Server_connection.shutdown t.server_connection + let shutdown t = + t.stopped <- true; + Server_connection.shutdown t.server_connection end open Runtime @@ -943,6 +949,21 @@ let test_shutdown_during_asynchronous_request () = writer_closed t ;; +let test_flush_response_before_shutdown () = + let reqd = ref None in + let request_handler reqd' = reqd := Some reqd' in + let t = create request_handler in + let request = Request.create `GET "/" ~headers:(Headers.encoding_fixed 0) in + read_request t request; + let reqd = Option.get !reqd in + let response = Response.create `OK in + let body = Reqd.respond_with_streaming ~flush_headers_immediately:true reqd response in + write_response t response; + Body.write_string body "hello world"; + shutdown t; + write_string t "hello world"; +;; + let tests = [ "initial reader state" , `Quick, test_initial_reader_state ; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof @@ -974,4 +995,5 @@ let tests = ; "response finished before body read", `Quick, test_response_finished_before_body_read ; "shutdown in request handler", `Quick, test_shutdown_in_request_handler ; "shutdown during asynchronous request", `Quick, test_shutdown_during_asynchronous_request + ; "flush response before shutdown", `Quick, test_flush_response_before_shutdown ] From 9d6cd86419b637fd15a9aacf165fd51f18637e2c Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sat, 22 May 2021 14:15:27 -0400 Subject: [PATCH 2/4] refactor shutdown handlers and fix test In the parent feature, some changes to the test runtime were made, but they weren't representative of what a normal runtime would do. This was due to a bug that was fixed in another branch that this is based off of. I reverted the changes to the runtime and reorganized the test to read more clearly. At the same time, I refactored `shutdown_reader` and `shutdown_writer`. We were checking `is_active` twice in `shutdown_writer`, and it wasn't clear if that was necessary. I also became curious of why we only wake the reader/writer if there is no request. Presumably, if our actions on the request cause it to wake the reader, then the wake call will be a no-op anyway. I reorganized the code to both make the two implementations more parallel and a bit more straightforward. This also meant we could drop the extra wakes in shutdown. --- lib/server_connection.ml | 19 ++++++------ lib_test/test_server_connection.ml | 49 ++++++++++++++---------------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/lib/server_connection.ml b/lib/server_connection.ml index a34a0ebb..20112113 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -138,17 +138,18 @@ let create ?(config=Config.default) ?(error_handler=default_error_handler) reque } let shutdown_reader t = - Reader.force_close t.reader; if is_active t - then Reqd.close_request_body (current_reqd_exn t) - else wakeup_reader t + then Reqd.close_request_body (current_reqd_exn t); + Reader.force_close t.reader; + wakeup_reader t let shutdown_writer t = - if is_active t then Reqd.flush_response_body (current_reqd_exn t); + if is_active t then ( + let reqd = current_reqd_exn t in + Reqd.close_request_body reqd; + Reqd.flush_response_body reqd); Writer.close t.writer; - if is_active t - then Reqd.close_request_body (current_reqd_exn t) - else wakeup_writer t + wakeup_writer t let error_code t = if is_active t @@ -157,9 +158,7 @@ let error_code t = let shutdown t = shutdown_reader t; - shutdown_writer t; - wakeup_reader t; - wakeup_writer t + shutdown_writer t let set_error_and_handle ?request t error = if is_active t then begin diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 004a0382..5f1b6f9b 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -53,7 +53,6 @@ end = struct ; write_loop : (unit -> unit) ; mutable read_unyield_hook : (unit -> unit) option ; mutable write_unyield_hook : (unit -> unit) option - ; mutable stopped : bool } let rec read_step t = @@ -64,13 +63,12 @@ end = struct | `Yield -> trace "reader: Yield"; t.read_operation <- `Yield; - if not t.stopped then - yield_reader t.server_connection (fun () -> - trace "reader: Yield callback"; - read_step t; - t.read_unyield_hook |> Option.iter (fun f -> - t.read_unyield_hook <- None; - f ())) + yield_reader t.server_connection (fun () -> + trace "reader: Yield callback"; + read_step t; + t.read_unyield_hook |> Option.iter (fun f -> + t.read_unyield_hook <- None; + f ())) | `Close -> trace "reader: Close"; t.read_operation <- `Close @@ -84,13 +82,12 @@ end = struct | `Yield -> t.write_operation <- `Yield; trace "writer: Yield"; - if not t.stopped then - yield_writer t.server_connection (fun () -> - trace "writer: Yield callback"; - write_step t; - t.write_unyield_hook |> Option.iter (fun f -> - t.write_unyield_hook <- None; - f ())) + yield_writer t.server_connection (fun () -> + trace "writer: Yield callback"; + write_step t; + t.write_unyield_hook |> Option.iter (fun f -> + t.write_unyield_hook <- None; + f ())) | `Close n -> trace "writer: Close"; t.write_operation <- `Close n @@ -106,7 +103,6 @@ end = struct ; write_loop = (fun () -> write_step (Lazy.force_val t)) ; read_unyield_hook = None ; write_unyield_hook = None - ; stopped = false }) in let t = Lazy.force_val t in @@ -165,9 +161,7 @@ end = struct let report_exn t = Server_connection.report_exn t.server_connection - let shutdown t = - t.stopped <- true; - Server_connection.shutdown t.server_connection + let shutdown t = Server_connection.shutdown t.server_connection end open Runtime @@ -950,18 +944,21 @@ let test_shutdown_during_asynchronous_request () = ;; let test_flush_response_before_shutdown () = - let reqd = ref None in - let request_handler reqd' = reqd := Some reqd' in - let t = create request_handler in let request = Request.create `GET "/" ~headers:(Headers.encoding_fixed 0) in - read_request t request; - let reqd = Option.get !reqd in let response = Response.create `OK in - let body = Reqd.respond_with_streaming ~flush_headers_immediately:true reqd response in + let continue = ref (fun () -> ()) in + let request_handler reqd = + let body = Reqd.respond_with_streaming ~flush_headers_immediately:true reqd response in + continue := (fun () -> + Body.write_string body "hello world"); + in + let t = create request_handler in + read_request t request; write_response t response; - Body.write_string body "hello world"; + !continue (); shutdown t; write_string t "hello world"; + connection_is_shutdown t ;; let tests = From 366cfbadd40af2e0dbe14432314995a5b3de406b Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Sat, 22 May 2021 18:30:35 -0400 Subject: [PATCH 3/4] demonstrate broken chunked behavior --- lib_test/test_server_connection.ml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml index 5f1b6f9b..86474879 100644 --- a/lib_test/test_server_connection.ml +++ b/lib_test/test_server_connection.ml @@ -945,19 +945,20 @@ let test_shutdown_during_asynchronous_request () = let test_flush_response_before_shutdown () = let request = Request.create `GET "/" ~headers:(Headers.encoding_fixed 0) in - let response = Response.create `OK in + let response = Response.create `OK ~headers:Headers.encoding_chunked in let continue = ref (fun () -> ()) in let request_handler reqd = let body = Reqd.respond_with_streaming ~flush_headers_immediately:true reqd response in continue := (fun () -> - Body.write_string body "hello world"); + Body.write_string body "hello world"; + Body.close_writer body); in let t = create request_handler in read_request t request; write_response t response; !continue (); shutdown t; - write_string t "hello world"; + write_string t "b\r\nhello world\r\n"; connection_is_shutdown t ;; From 085476a40406b7f5b8002e45b7586e146303b5eb Mon Sep 17 00:00:00 2001 From: Doug Patti Date: Fri, 18 Jun 2021 15:39:57 -0400 Subject: [PATCH 4/4] comment on a long-standing behavior This is not new, I only just noticed it now. Removing the `close_request_body` line does not break any tests. I wonder if it meant to close the response body? --- lib/server_connection.ml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/server_connection.ml b/lib/server_connection.ml index 74b5e83d..839a10b3 100644 --- a/lib/server_connection.ml +++ b/lib/server_connection.ml @@ -147,6 +147,9 @@ let shutdown_reader t = let shutdown_writer t = if is_active t then ( let reqd = current_reqd_exn t in + (* XXX(dpatti): I'm not sure I understand why we close the *request* body + here. Maybe we can write a test such that removing this line causes it to + fail? *) Reqd.close_request_body reqd; Reqd.flush_response_body reqd); Writer.close t.writer;