Process requests after write EOF

[dhouse-js]

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.

[seliopou]

Can you rebase this?

[dhouse-js]

Done.

The diff still has spurious stuff in it. I think this is because refactor-request-queue is behind master by a few commits in the main httpaf repo. Perhaps that branch could be merged with httpaf/master?

[seliopou]

Sorry, I thought this was based off master but it's based off the #172 branch. I'll talk to @dpatti to see if that is good, and then all the other PRs will follow.

[dpatti]

Rebased to master

[dpatti]

I think I'm seeing a problem here. Consider a change like this:

diff --git a/lib_test/test_server_connection.ml b/lib_test/test_server_connection.ml
index d47c62a..f55af7a 100644
--- a/lib_test/test_server_connection.ml
+++ b/lib_test/test_server_connection.ml
@@ -913,6 +913,8 @@ let test_can_read_more_requests_after_write_eof () =
   reqd := None;
   reader_ready t;
   read_request t request;
+  Reqd.respond_with_streaming (Option.get !reqd) response ~flush_headers_immediately:true
+  |> (ignore : [ `write ] Body.t -> unit);
   Alcotest.(check bool) "request handler fired" true (Option.is_some !reqd)
 ;;

Roughly, imagine that this is called in every request handler like it would in a real application. This will raise with [failure] cannot write to closed writer, which comes from Faraday because respond_with_streaming (and likewise, respond_with_string) both try to write to t.writer, which is already closed. I think we could repro this trivially already (edit: yes), though: if your request handler is asynchronous, and something shuts the writer down before it runs (which I think can only happen with an explicit shutdown call), then it would raise for the same reasons. In this case, though, we'd always raise because we're willingly calling the request handler even though the writer is closed.

I'm not sure what the ideal behavior here is.

[dhouse-js]

FWIW, when developing my application I ran into the exact case you're describing -- a response handler needs some time to decide how to respond, and gets an exception when it finally does respond because the writer has been closed in the mean time.

It seems to me that the only options are:

1. Keep the current behaviour: raise if a write has already happened.
2. Silently drop responses if the writer is already closed.
3. Return a result type that indicates whether the response was successfully written to the writer or not.

I went with (2) in my application -- i.e., I wrapped all of the respond functions to catch this exception. This felt acceptable to me because servers don't typically care if their responses were actually sent to the client or not. And of course, even if we do successfully put the response into the writer, that is no guarantee that the bytes will be able to be delivered to the network and received by the client.

(2) does have the disadvantage that it destroys information, but perhaps we could expose a Reqd.writer_is_open function that allows the caller to recover it should they wish.

It's worth noting that, even with (2), you have a further choice to make when it comes to respond_with_streaming: do you still deliver a Body.t to the client, even though no content in that body will ever be sent to the writer? In my application, I drew the line here, and instead wrapped respond_with_streaming to have a return type of [`write] Body.t option.

I can write a separate PR for this should people wish.