[HTTP/3] Handle all QUIC exceptions in H/3 stream #116851
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances HTTP/3 error handling by catching all QUIC transport exceptions and surfacing them as HttpRequestException to clients.
- Added a functional test to verify that client observes a propagated QUIC error when the server aborts the connection.
- Introduced a
catch (QuicException)block inHttp3RequestStreamto abort the connection and throw a specificHttpRequestException. - Added a new resource string
net_http_http3_connection_quic_errorfor QUIC transport failures.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs | New test ServerKillsQuicConnection_ClientPropagatesException for QUIC exception propagation |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs | Catch QuicException, abort connection, and throw HttpRequestException |
| src/libraries/System.Net.Http/src/Resources/Strings.resx | Added net_http_http3_connection_quic_error resource string |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs:326
- Using
HttpRequestError.Unknownis generic for QUIC transport failures. Consider defining and using a more specific enum value (e.g.,QuicTransportError) so consumers can distinguish QUIC errors from other failures.
throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_http3_connection_quic_error, ex);
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
Show resolved
Hide resolved
| { | ||
| // Any other QuicException means transport error, and should be treated as a connection failure. | ||
| _connection.Abort(ex); | ||
| throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_http3_connection_quic_error, ex); |
There was a problem hiding this comment.
If this is a transport error, shouldn't we use HttpRequestError.ConnectionError instead? Moreover, are we sure we don't have an ApplicationErrorCode here?
There was a problem hiding this comment.
shouldn't we use HttpRequestError.ConnectionError instead?
I didn't use ConnectionError because of this in the docs:
while connecting to the remote endpoint
But I'm not oppose to using it and maybe tweaking the docs to not to be exclusive to connection establishment.
Moreover, are we sure we don't have an ApplicationErrorCode here?
Yes, it's present only in case of ConnectionAborted / StreamAborted, which are handled above. The application code has to be explicitly sent by the peer, which is specified by QUIC RFC, so there's no chance we would have it in any other cases (meaning QuicError).
There was a problem hiding this comment.
Ok, all makes sense now, I'm good with the current handling then.
maybe tweaking the docs
This leads too far, e.g. it should be then used in other cases or maybe we should rather consider adding a new enum member.
|
HTTP outerloop failures are unrelated: |
Fixes #116845