Server checking if stream closed

[ShawnROGrady]

H2.Reqd fails various operations (respond_with_streaming, schedule_trailers, others) if the client closes the stream, but currently it doesn't look like there's a way for the for a server implementation to avoid/handle these failures.

I would think the clearest way to propagate cancellation would be to make ~request_handler for the lwt/async server implementations accept a promise which is resolved/rejected when the stream is closed. However that might require a version bump, so it might be simpler to just raise an exception other than Failure (such as currently the case with schedule_trailers) or Assert_failure (currently the case with respond_with_streaming) when trying to respond on a closed stream. Making the server explicitly catch a Failure or Assert_failure seems like a bad idea, since we would unintentionally catch actual unexpected errors, but this seems like an expected case that a server should be able to gracefully handle.

I'm a huge fan of this project, and thank you for all of your work (I'm fairly new to ocaml and have found this project incredibly approachable), so sorry if this is something that's already possible that I missed.

[anmonteiro]

Thanks for opening an issue, and for the kind words.

I agree with you. These should definitely be handled better. A few thoughts:

1. Generally we should only use assert false in cases that are unreachable (e.g. we can't respond_with_streaming on a Reqd.t that hasn't been surfaced by the API)
2. I didn't do a good job of making sure (1.) holds. In fact, there might be a few other assert falses that are placeholders for TODO items.
3. I feel like there are a few options here: a. instead of returning unit, we could return (unit, stream_state) result for some values of stream_state like Closed, etc b. we could create an exception Stream_closed and fail in these cases
4. Whatever we end up doing, the language should probably make sense for HTTP/1.X, as I've purposely kept the Reqd API close to http/af so that it's easier to abstract away in piaf

Both alternatives above are technically breaking changes, so I think we get to pick. Do you see other alternatives?

[ShawnROGrady]

100% agreed on point 1, I think something like an assert false or failwith is a good way to indicate a failure due to something reaching a theoretically unreachable state.

For point 3, I personally like result since it makes it clear from the API that the operation may fail in expected cases, but realistically I understand that raising an exception may be more ergonomic. Mainly, I would think it might be a good idea to go with option b (using an exception) until the API finalized.

For your last point, this is a bit difficult for me since I don't have much low-level experience of directly interacting with transport protocols. I mainly use go and HTTP/1 at my day job (whose server-side request cancellation detection appears to come from a and b), and I know .NET has something similar (but I have no experience with), so to me the approach I think would be easiest for consumers would be to make this property accessible from the request context. Either through H2.Reqd.t or through a promise passed to ~request_handler. Mainly such an approach would allow the server to propagate cancellation down the stack, such as in a situation where the server makes another outgoing request, but from my understanding this would definitely require the most changes to the server-side logic.

To me, adding something like exception Stream_closed seems like the best solution for now to allow servers to gracefully handle while not blocking future versions from solving the problem a different way.

EDIT: My main thoughts come from me not being fully comfortable with what the API should look like. To me, using exception Stream_closed doesn't feel like the ideal solution, but it would allow servers to gracefully handle this case currently while allowing a different solution to be added in the future.