HTTP/3 server: send a reset when do_send errors out #3326
Conversation
The current behavior of http3/server.c::do_send() is to close the stream, there is thus no way for the client to tell that there might be an issue with the response. The code as proposed in this commit does present the issue noted in quicwg/base-drafts#3300: a re-ordering of packets might result in a worse end user experience compared to a TCP based protocol.
|
CI failed on the h3 fuzzer, i will take a look. |
My understanding is that the fuzzer doesn't take into account the possibility that there was a reset sent. Before going ahead and fixing it, @kazuho what do you think of the approach? I considered setting a flag on the stream in order to sent a delayed reset instead, but that seemed trickier to test and understand, so that made me think that the approach above is more sensible. |
There was a problem hiding this comment.
@deweerdt I like the direction of the PR.
In the long run, we have the hope to rely on Reliable Stream Resets to handle the problem; it would likely be implemented by web browsers as it is a dependency of WebTransport.
For clients that do not implement that QUIC extension, resetting would be fine.
| /* TODO consider how to forward error, pending resolution of https://github.com/quicwg/base-drafts/issues/3300 */ | ||
| /* send a reset once the data stream, TODO delay reset: https://github.com/quicwg/base-drafts/issues/3300 */ | ||
| quicly_reset_stream(stream->quic, QUICLY_ERROR_FROM_APPLICATION_ERROR_CODE(0)); | ||
| break; |
There was a problem hiding this comment.
I do not think we want to continue running to the end of the function to call finalize_do_send, as the function assumes that the there is still something to be sent from the stream.
The correct way of error-closing a stream is to call shutdown_stream and do just that.
|
One downside that I noticed with the proposed approach is that there is no guarantee that even the HTTP headers would be received by the client. Depending on if the bytes have been emitted onto the wire or depending on when the client receives the HTTP headers or the reset, there is chance that the HTTP client just sees the reset. I still like this change but it's worth noting that there is such a risk. |
I agree, that's what i tried to convey via:
We could go down the path of waiting for the stream to be acked before emitting the RST but that seemed to be a large change. |
The current behavior of http3/server.c::do_send() is to close the stream, there is thus no way for the client to tell that there might be an issue with the response.
The code as proposed in this commit does present the issue noted in quicwg/base-drafts#3300: a re-ordering of packets might result in a worse end user experience compared to a TCP based protocol.