Flush response before shutting down #208
Conversation
dpatti
force-pushed
the
flush-response-before-shutting-down
branch
from
6a52341 to
55d806c
Compare
dpatti
force-pushed
the
flush-response-before-shutting-down
branch
from
55d806c to
084aef7
Compare
There was a problem hiding this comment.
I rebased to fix history and added a commit:
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_readerandshutdown_writer.
We were checkingis_activetwice inshutdown_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_test/test_server_connection.ml
Outdated
| t.read_unyield_hook |> Option.iter (fun f -> | ||
| t.read_unyield_hook <- None; | ||
| f ())) | ||
| if not t.stopped then |
There was a problem hiding this comment.
This is a little strange – if we need to change the runtime to make the test pass, I suspect that something is wrong with shutdown. I'm going to make a separate test to see what happens first.
There was a problem hiding this comment.
Okay, I rebased this off of another branch that I believe fixes an issue exposed here.
dpatti
force-pushed
the
flush-response-before-shutting-down
branch
from
084aef7 to
55c100e
Compare
|
This can also raise if we write more to the body after the shutdown. The shutdown will flush, but then the write loop will eventually realize that there is more to write and will try to flush it. We could try to close the response body inside the shutdown, but chunked responses will still fail because they try to write on the close. In fact, chunked responses already don't work for this, it just wasn't apparent because we didn't close the body. This is also related to #209. |
|
Oh, wow, it looks like when I merged the base and deleted the branch too quickly, it closed this PR automatically? That's surprising behavior. |
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.
dpatti
force-pushed
the
flush-response-before-shutting-down
branch
from
dd4ff5d to
366cfba
Compare
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?
Server_connection.shutdownimmediately callsWriter.close. But we should flush the response body in case the user has written something to the body and not yet flushed the body.You could argue this is not needed because the user should be careful to flush their body before shutting down. In the structure of my program, this is hard to ensure. I expose a function to shut down the HTTP server, which calls Server_connection.shutdown on all live connections. It's hard for that function to know what the current state of all of the response handlers are.