From e2e9df7a7bccb04a5d57e87ecd04a2522f249ac7 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Thu, 14 Jul 2022 20:45:31 +0200 Subject: [PATCH] Implement HttpProtocolException for HTTP/3 (#72095) * Replace Http3ProtocolException with HttpProtocolException * Correctly process incoming protocol errors * Fix HTTP3 stress * Add some tests * Remove formating changes * Code review feedback * Fix build * Throw HttpProtocolException from content stream as well * Add test for throwing when reading the content stream --- .../Net/Http/Http3LoopbackConnection.cs | 5 +- .../src/Resources/Strings.resx | 9 ++ .../src/System.Net.Http.csproj | 2 - .../System/Net/Http/HttpProtocolException.cs | 39 +++++- .../SocketsHttpHandler/Http2Connection.cs | 13 +- .../SocketsHttpHandler/Http3Connection.cs | 55 ++++---- .../Http3ConnectionException.cs | 24 ---- .../Http3ProtocolException.cs | 56 -------- .../SocketsHttpHandler/Http3RequestStream.cs | 50 +++++--- .../HttpClientHandlerTest.Http2.cs | 4 +- .../HttpClientHandlerTest.Http3.cs | 121 +++++++++++++++++- .../HttpStress/ClientOperations.cs | 31 +---- .../src/System/Net/Quic/QuicException.cs | 3 + 13 files changed, 246 insertions(+), 166 deletions(-) delete mode 100644 src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ConnectionException.cs delete mode 100644 src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ProtocolException.cs diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index c64ea03a392..953c912114c 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs @@ -84,10 +84,7 @@ public override async ValueTask DisposeAsync() #endif } - public async Task CloseAsync(long errorCode) - { - await _connection.CloseAsync(errorCode).ConfigureAwait(false); - } + public Task CloseAsync(long errorCode) => _connection.CloseAsync(errorCode).AsTask(); public async ValueTask OpenUnidirectionalStreamAsync() { diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index 3ac957f5d81..5a250aaa584 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -408,9 +408,15 @@ The HTTP/2 server sent invalid data on the connection. HTTP/2 error code '{0}' (0x{1}). + + The HTTP/2 server closed the connection. HTTP/2 error code '{0}' (0x{1}). + The HTTP/2 server reset the stream. HTTP/2 error code '{0}' (0x{1}). + + The HTTP/3 server reset the stream. HTTP/3 error code '{0}' (0x{1}). + An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. @@ -477,6 +483,9 @@ The HTTP/3 server sent invalid data on the connection. HTTP/3 error code '{0}' (0x{1}). + + The HTTP/3 server closed the connection. HTTP/3 error code '{0}' (0x{1}). + The server is unable to process the request using the current HTTP version and indicates the request should be retried on an older HTTP version. diff --git a/src/libraries/System.Net.Http/src/System.Net.Http.csproj b/src/libraries/System.Net.Http/src/System.Net.Http.csproj index d8780e26ccc..98d1da69d26 100644 --- a/src/libraries/System.Net.Http/src/System.Net.Http.csproj +++ b/src/libraries/System.Net.Http/src/System.Net.Http.csproj @@ -181,8 +181,6 @@ - - diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpProtocolException.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpProtocolException.cs index 5d3e914aeac..15d9eae82b0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpProtocolException.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpProtocolException.cs @@ -41,9 +41,21 @@ internal static HttpProtocolException CreateHttp2StreamException(Http2ProtocolEr return new HttpProtocolException((long)protocolError, message, null); } - internal static HttpProtocolException CreateHttp2ConnectionException(Http2ProtocolErrorCode protocolError) + internal static HttpProtocolException CreateHttp2ConnectionException(Http2ProtocolErrorCode protocolError, string? message = null) { - string message = SR.Format(SR.net_http_http2_connection_error, GetName(protocolError), ((int)protocolError).ToString("x")); + message = SR.Format(message ?? SR.net_http_http2_connection_error, GetName(protocolError), ((int)protocolError).ToString("x")); + return new HttpProtocolException((long)protocolError, message, null); + } + + internal static HttpProtocolException CreateHttp3StreamException(Http3ErrorCode protocolError) + { + string message = SR.Format(SR.net_http_http3_stream_error, GetName(protocolError), ((int)protocolError).ToString("x")); + return new HttpProtocolException((long)protocolError, message, null); + } + + internal static HttpProtocolException CreateHttp3ConnectionException(Http3ErrorCode protocolError, string? message = null) + { + message = SR.Format(message ?? SR.net_http_http3_connection_error, GetName(protocolError), ((int)protocolError).ToString("x")); return new HttpProtocolException((long)protocolError, message, null); } @@ -67,6 +79,29 @@ code switch Http2ProtocolErrorCode.Http11Required => "HTTP_1_1_REQUIRED", _ => "(unknown error)", }; + + private static string GetName(Http3ErrorCode code) => + // These strings come from the H3 spec and should not be localized. + code switch + { + Http3ErrorCode.NoError => "H3_NO_ERROR", + Http3ErrorCode.ProtocolError => "H3_GENERAL_PROTOCOL_ERROR", + Http3ErrorCode.InternalError => "H3_INTERNAL_ERROR", + Http3ErrorCode.StreamCreationError => "H3_STREAM_CREATION_ERROR", + Http3ErrorCode.ClosedCriticalStream => "H3_CLOSED_CRITICAL_STREAM", + Http3ErrorCode.UnexpectedFrame => "H3_FRAME_UNEXPECTED", + Http3ErrorCode.FrameError => "H3_FRAME_ERROR", + Http3ErrorCode.ExcessiveLoad => "H3_EXCESSIVE_LOAD", + Http3ErrorCode.IdError => "H3_ID_ERROR", + Http3ErrorCode.SettingsError => "H3_SETTINGS_ERROR", + Http3ErrorCode.MissingSettings => "H3_MISSING_SETTINGS", + Http3ErrorCode.RequestRejected => "H3_REQUEST_REJECTED", + Http3ErrorCode.RequestCancelled => "H3_REQUEST_CANCELLED", + Http3ErrorCode.RequestIncomplete => "H3_REQUEST_INCOMPLETE", + Http3ErrorCode.ConnectError => "H3_CONNECT_ERROR", + Http3ErrorCode.VersionFallback => "H3_VERSION_FALLBACK", + _ => "(unknown error)" + }; #endif } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index 30c8761de4e..e026419270f 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -170,7 +170,8 @@ public Http2Connection(HttpConnectionPool pool, Stream stream) if (NetEventSource.Log.IsEnabled()) TraceConnection(_stream); - static long TimeSpanToMs(TimeSpan value) { + static long TimeSpanToMs(TimeSpan value) + { double milliseconds = value.TotalMilliseconds; return (long)(milliseconds > int.MaxValue ? int.MaxValue : milliseconds); } @@ -486,7 +487,7 @@ private async Task ProcessIncomingFramesAsync() if (frameHeader.Type == FrameType.GoAway) { var (_, errorCode) = ReadGoAwayFrame(frameHeader); - ThrowProtocolError(errorCode); + ThrowProtocolError(errorCode, SR.net_http_http2_connection_close); } else { @@ -1045,7 +1046,7 @@ private void ProcessGoAwayFrame(FrameHeader frameHeader) var (lastStreamId, errorCode) = ReadGoAwayFrame(frameHeader); Debug.Assert(lastStreamId >= 0); - Exception resetException = HttpProtocolException.CreateHttp2ConnectionException(errorCode); + Exception resetException = HttpProtocolException.CreateHttp2ConnectionException(errorCode, SR.net_http_http2_connection_close); // There is no point sending more PING frames for RTT estimation: _rttEstimator.OnGoAwayReceived(); @@ -1258,7 +1259,7 @@ private async Task ProcessOutgoingFramesAsync() Debug.Assert(sizeof(long) == FrameHeader.PingLength); Span span = writeBuffer.Span; - FrameHeader.WriteTo(span, FrameHeader.PingLength, FrameType.Ping, state.isAck ? FrameFlags.Ack: FrameFlags.None, streamId: 0); + FrameHeader.WriteTo(span, FrameHeader.PingLength, FrameType.Ping, state.isAck ? FrameFlags.Ack : FrameFlags.None, streamId: 0); BinaryPrimitives.WriteInt64BigEndian(span.Slice(FrameHeader.Size), state.pingContent); return true; @@ -2140,7 +2141,7 @@ private void VerifyKeepAlive() ThrowProtocolError(Http2ProtocolErrorCode.ProtocolError); [DoesNotReturn] - private static void ThrowProtocolError(Http2ProtocolErrorCode errorCode) => - throw HttpProtocolException.CreateHttp2ConnectionException(errorCode); + private static void ThrowProtocolError(Http2ProtocolErrorCode errorCode, string? message = null) => + throw HttpProtocolException.CreateHttp2ConnectionException(errorCode, message); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs index e22e3502f6c..bfebc041933 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs @@ -232,13 +232,11 @@ public async Task SendAsync(HttpRequestMessage request, lon return await responseTask.ConfigureAwait(false); } - catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted) + catch (QuicException ex) when (ex.QuicError == QuicError.OperationAborted) { - Debug.Assert(ex.ApplicationErrorCode.HasValue); - - // This will happen if we aborted _connection somewhere. - Abort(ex); - throw new HttpRequestException(SR.Format(SR.net_http_http3_connection_error, ex.ApplicationErrorCode.Value), ex, RequestRetryType.RetryOnConnectionFailure); + // This will happen if we aborted _connection somewhere and we have pending OpenOutboundStreamAsync call. + Debug.Assert(_abortException is not null); + throw new HttpRequestException(SR.net_http_client_execution_error, _abortException, RequestRetryType.RetryOnConnectionFailure); } finally { @@ -271,7 +269,7 @@ internal Exception Abort(Exception abortException) // Stop sending requests to this connection. _pool.InvalidateHttp3Connection(this); - Http3ErrorCode connectionResetErrorCode = (abortException as Http3ProtocolException)?.ErrorCode ?? Http3ErrorCode.InternalError; + long connectionResetErrorCode = (abortException as HttpProtocolException)?.ErrorCode ?? (long)Http3ErrorCode.InternalError; lock (SyncObj) { @@ -423,6 +421,13 @@ private async Task AcceptStreamsAsync() { // Shutdown initiated by us, no need to abort. } + catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted) + { + Debug.Assert(ex.ApplicationErrorCode.HasValue); + Http3ErrorCode code = (Http3ErrorCode)ex.ApplicationErrorCode.Value; + + Abort(HttpProtocolException.CreateHttp3ConnectionException(code, SR.net_http_http3_connection_close)); + } catch (Exception ex) { Abort(ex); @@ -443,7 +448,7 @@ await using (stream.ConfigureAwait(false)) if (stream.CanWrite) { // Server initiated bidirectional streams are either push streams or extensions, and we support neither. - throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); } buffer = new ArrayBuffer(initialSize: 32, usePool: true); @@ -478,7 +483,7 @@ await using (stream.ConfigureAwait(false)) if (Interlocked.Exchange(ref _haveServerControlStream, 1) != 0) { // A second control stream has been received. - throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); } // Discard the stream type header. @@ -494,7 +499,7 @@ await using (stream.ConfigureAwait(false)) if (Interlocked.Exchange(ref _haveServerQpackDecodeStream, 1) != 0) { // A second QPack decode stream has been received. - throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); } // The stream must not be closed, but we aren't using QPACK right now -- ignore. @@ -505,7 +510,7 @@ await using (stream.ConfigureAwait(false)) if (Interlocked.Exchange(ref _haveServerQpackEncodeStream, 1) != 0) { // A second QPack encode stream has been received. - throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); } // We haven't enabled QPack in our SETTINGS frame, so we shouldn't receive any meaningful data here. @@ -516,7 +521,7 @@ await using (stream.ConfigureAwait(false)) case (byte)Http3StreamType.Push: // We don't support push streams. // Because no maximum push stream ID was negotiated via a MAX_PUSH_ID frame, server should not have sent this. Abort the connection with H3_ID_ERROR. - throw new Http3ConnectionException(Http3ErrorCode.IdError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.IdError); default: // Unknown stream type. Per spec, these must be ignored and aborted but not be considered a connection-level error. @@ -573,12 +578,12 @@ private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffe if (frameType == null) { // Connection closed prematurely, expected SETTINGS frame. - throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ClosedCriticalStream); } if (frameType != Http3FrameType.Settings) { - throw new Http3ConnectionException(Http3ErrorCode.MissingSettings); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.MissingSettings); } await ProcessSettingsFrameAsync(payloadLength).ConfigureAwait(false); @@ -596,7 +601,7 @@ private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffe break; case Http3FrameType.Settings: // If an endpoint receives a second SETTINGS frame on the control stream, the endpoint MUST respond with a connection error of type H3_FRAME_UNEXPECTED. - throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.UnexpectedFrame); case Http3FrameType.Headers: // Servers should not send these frames to a control stream. case Http3FrameType.Data: case Http3FrameType.MaxPushId: @@ -604,11 +609,11 @@ private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffe case Http3FrameType.ReservedHttp2Ping: case Http3FrameType.ReservedHttp2WindowUpdate: case Http3FrameType.ReservedHttp2Continuation: - throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.UnexpectedFrame); case Http3FrameType.PushPromise: case Http3FrameType.CancelPush: // Because we haven't sent any MAX_PUSH_ID frame, it is invalid to receive any push-related frames as they will all reference a too-large ID. - throw new Http3ConnectionException(Http3ErrorCode.IdError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.IdError); case null: // End of stream reached. If we're shutting down, stop looping. Otherwise, this is an error (this stream should not be closed for life of connection). bool shuttingDown; @@ -618,7 +623,7 @@ private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffe } if (!shuttingDown) { - throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ClosedCriticalStream); } return; default: @@ -650,7 +655,7 @@ async ValueTask<(Http3FrameType? frameType, long payloadLength)> ReadFrameEnvelo else { // Our buffer has partial frame data in it but not enough to complete the read: bail out. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } } @@ -678,7 +683,7 @@ async ValueTask ProcessSettingsFrameAsync(long settingsPayloadLength) else { // Our buffer has partial frame data in it but not enough to complete the read: bail out. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } } @@ -688,7 +693,7 @@ async ValueTask ProcessSettingsFrameAsync(long settingsPayloadLength) { // An integer was encoded past the payload length. // A frame payload that contains additional bytes after the identified fields or a frame payload that terminates before the end of the identified fields MUST be treated as a connection error of type H3_FRAME_ERROR. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } buffer.Discard(bytesRead); @@ -704,7 +709,7 @@ async ValueTask ProcessSettingsFrameAsync(long settingsPayloadLength) case Http3SettingType.ReservedHttp2MaxFrameSize: // Per https://tools.ietf.org/html/draft-ietf-quic-http-31#section-7.2.4.1 // these settings IDs are reserved and must never be sent. - throw new Http3ConnectionException(Http3ErrorCode.SettingsError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.SettingsError); } } } @@ -726,7 +731,7 @@ async ValueTask ProcessGoAwayFrameAsync(long goawayPayloadLength) else { // Our buffer has partial frame data in it but not enough to complete the read: bail out. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } } @@ -734,7 +739,7 @@ async ValueTask ProcessGoAwayFrameAsync(long goawayPayloadLength) if (bytesRead != goawayPayloadLength) { // Frame contains unknown extra data after the integer. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } OnServerGoAway(firstRejectedStreamId); @@ -755,7 +760,7 @@ async ValueTask SkipUnknownPayloadAsync(Http3FrameType frameType, long payloadLe else { // Our buffer has partial frame data in it but not enough to complete the read: bail out. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ConnectionException.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ConnectionException.cs deleted file mode 100644 index dbb22d36a79..00000000000 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ConnectionException.cs +++ /dev/null @@ -1,24 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.Serialization; -using System.Runtime.Versioning; - -namespace System.Net.Http -{ - [Serializable] - [SupportedOSPlatform("windows")] - [SupportedOSPlatform("linux")] - [SupportedOSPlatform("macos")] - internal sealed class Http3ConnectionException : Http3ProtocolException - { - public Http3ConnectionException(Http3ErrorCode errorCode) - : base(SR.Format(SR.net_http_http3_connection_error, GetName(errorCode), ((long)errorCode).ToString("x")), errorCode) - { - } - - private Http3ConnectionException(SerializationInfo info, StreamingContext context) : base(info, context) - { - } - } -} diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ProtocolException.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ProtocolException.cs deleted file mode 100644 index c3731643a23..00000000000 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3ProtocolException.cs +++ /dev/null @@ -1,56 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.Serialization; -using System.Runtime.Versioning; - -namespace System.Net.Http -{ - [Serializable] - [SupportedOSPlatform("windows")] - [SupportedOSPlatform("linux")] - [SupportedOSPlatform("macos")] - internal class Http3ProtocolException : Exception - { - public Http3ErrorCode ErrorCode { get; } - - protected Http3ProtocolException(string message, Http3ErrorCode errorCode) : base(message) - { - ErrorCode = errorCode; - } - - protected Http3ProtocolException(SerializationInfo info, StreamingContext context) : base(info, context) - { - ErrorCode = (Http3ErrorCode)info.GetUInt32(nameof(ErrorCode)); - } - - public override void GetObjectData(SerializationInfo info, StreamingContext context) - { - info.AddValue(nameof(ErrorCode), (uint)ErrorCode); - base.GetObjectData(info, context); - } - - protected static string GetName(Http3ErrorCode errorCode) => - // These strings come from the H3 spec and should not be localized. - errorCode switch - { - Http3ErrorCode.NoError => "H3_NO_ERROR (0x100)", - Http3ErrorCode.ProtocolError => "H3_GENERAL_PROTOCOL_ERROR (0x101)", - Http3ErrorCode.InternalError => "H3_INTERNAL_ERROR (0x102)", - Http3ErrorCode.StreamCreationError => "H3_STREAM_CREATION_ERROR (0x103)", - Http3ErrorCode.ClosedCriticalStream => "H3_CLOSED_CRITICAL_STREAM (0x104)", - Http3ErrorCode.UnexpectedFrame => "H3_FRAME_UNEXPECTED (0x105)", - Http3ErrorCode.FrameError => "H3_FRAME_ERROR (0x106)", - Http3ErrorCode.ExcessiveLoad => "H3_EXCESSIVE_LOAD (0x107)", - Http3ErrorCode.IdError => "H3_ID_ERROR (0x108)", - Http3ErrorCode.SettingsError => "H3_SETTINGS_ERROR (0x109)", - Http3ErrorCode.MissingSettings => "H3_MISSING_SETTINGS (0x10A)", - Http3ErrorCode.RequestRejected => "H3_REQUEST_REJECTED (0x10B)", - Http3ErrorCode.RequestCancelled => "H3_REQUEST_CANCELLED (0x10C)", - Http3ErrorCode.RequestIncomplete => "H3_REQUEST_INCOMPLETE (0x10D)", - Http3ErrorCode.ConnectError => "H3_CONNECT_ERROR (0x10F)", - Http3ErrorCode.VersionFallback => "H3_VERSION_FALLBACK (0x110)", - _ => "(unknown error)" - }; - } -} diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs index 3f16d194d0c..d5a4e7596f3 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Net.Http.Headers; using System.Net.Quic; @@ -231,8 +232,9 @@ public async Task SendAsync(CancellationToken cancellationT catch (QuicException ex) when (ex.QuicError == QuicError.StreamAborted) { Debug.Assert(ex.ApplicationErrorCode.HasValue); + Http3ErrorCode code = (Http3ErrorCode)ex.ApplicationErrorCode.Value; - switch ((Http3ErrorCode)ex.ApplicationErrorCode.Value) + switch (code) { case Http3ErrorCode.VersionFallback: // The server is requesting us fall back to an older HTTP version. @@ -244,14 +246,16 @@ public async Task SendAsync(CancellationToken cancellationT default: // Our stream was reset. - Exception? abortException = _connection.AbortException; - throw new HttpRequestException(SR.net_http_client_execution_error, abortException ?? ex); + throw new HttpRequestException(SR.net_http_client_execution_error, _connection.AbortException ?? HttpProtocolException.CreateHttp3StreamException(code)); } } catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted) { // Our connection was reset. Start shutting down the connection. - Exception abortException = _connection.Abort(ex); + Debug.Assert(ex.ApplicationErrorCode.HasValue); + Http3ErrorCode code = (Http3ErrorCode)ex.ApplicationErrorCode.Value; + + Exception abortException = _connection.Abort(HttpProtocolException.CreateHttp3ConnectionException(code, SR.net_http_http3_connection_close)); throw new HttpRequestException(SR.net_http_client_execution_error, abortException); } // It is possible for user's Content code to throw an unexpected OperationCanceledException. @@ -269,7 +273,7 @@ public async Task SendAsync(CancellationToken cancellationT throw new HttpRequestException(SR.net_http_request_aborted, ex, RequestRetryType.RetryOnConnectionFailure); } } - catch (Http3ConnectionException ex) + catch (HttpProtocolException ex) { // A connection-level protocol error has occurred on our stream. _connection.Abort(ex); @@ -794,12 +798,12 @@ private async ValueTask<(Http3FrameType? frameType, long payloadLength)> ReadFra case Http3FrameType.ReservedHttp2Ping: case Http3FrameType.ReservedHttp2WindowUpdate: case Http3FrameType.ReservedHttp2Continuation: - throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.UnexpectedFrame); case Http3FrameType.PushPromise: case Http3FrameType.CancelPush: // Because we haven't sent any MAX_PUSH_ID frames, any of these push-related // frames that the server sends will have an out-of-range push ID. - throw new Http3ConnectionException(Http3ErrorCode.IdError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.IdError); default: // Unknown frame types should be skipped. await SkipUnknownPayloadAsync(payloadLength, cancellationToken).ConfigureAwait(false); @@ -883,7 +887,7 @@ private void GetStaticQPackHeader(int index, out HeaderDescriptor descriptor, ou if (!HeaderDescriptor.TryGetStaticQPackHeader(index, out descriptor, out knownValue)) { if (NetEventSource.Log.IsEnabled()) Trace($"Response contains invalid static header index '{index}'."); - throw new Http3ConnectionException(Http3ErrorCode.ProtocolError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ProtocolError); } } @@ -899,13 +903,13 @@ private void OnHeader(int? staticIndex, HeaderDescriptor descriptor, string? sta if (!descriptor.Equals(KnownHeaders.PseudoStatus)) { if (NetEventSource.Log.IsEnabled()) Trace($"Received unknown pseudo-header '{descriptor.Name}'."); - throw new Http3ConnectionException(Http3ErrorCode.ProtocolError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ProtocolError); } if (_headerState != HeaderState.StatusHeader) { if (NetEventSource.Log.IsEnabled()) Trace("Received extra status header."); - throw new Http3ConnectionException(Http3ErrorCode.ProtocolError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ProtocolError); } int statusCode; @@ -996,7 +1000,7 @@ int ParseStatusCode(int? index, string value) { case HeaderState.StatusHeader: if (NetEventSource.Log.IsEnabled()) Trace($"Received headers without :status."); - throw new Http3ConnectionException(Http3ErrorCode.ProtocolError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.ProtocolError); case HeaderState.ResponseHeaders when descriptor.HeaderType.HasFlag(HttpHeaderType.Content): _response!.Content!.Headers.TryAddWithoutValidation(descriptor, headerValue); break; @@ -1034,7 +1038,7 @@ private async ValueTask SkipUnknownPayloadAsync(long payloadLength, Cancellation else { // Our buffer has partial frame data in it but not enough to complete the read: bail out. - throw new Http3ConnectionException(Http3ErrorCode.FrameError); + throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.FrameError); } } @@ -1185,21 +1189,29 @@ private async ValueTask ReadResponseContentAsync(HttpResponseMessage respon } } + [DoesNotReturn] private void HandleReadResponseContentException(Exception ex, CancellationToken cancellationToken) { switch (ex) { - case QuicException e when (e.QuicError == QuicError.StreamAborted || e.QuicError == QuicError.OperationAborted): - // Peer or user aborted the stream - throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, ex)); + case QuicException e when (e.QuicError == QuicError.StreamAborted): + // Peer aborted the stream + Debug.Assert(e.ApplicationErrorCode.HasValue); + throw HttpProtocolException.CreateHttp3StreamException((Http3ErrorCode)e.ApplicationErrorCode.Value); + case QuicException e when (e.QuicError == QuicError.ConnectionAborted): // Our connection was reset. Start aborting the connection. - Exception abortException = _connection.Abort(ex); - throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, abortException)); - case Http3ConnectionException: + Debug.Assert(e.ApplicationErrorCode.HasValue); + HttpProtocolException exception = HttpProtocolException.CreateHttp3ConnectionException((Http3ErrorCode)e.ApplicationErrorCode.Value, SR.net_http_http3_connection_close); + _connection.Abort(exception); + throw exception; + + case HttpProtocolException: // A connection-level protocol error has occurred on our stream. _connection.Abort(ex); - throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, ex)); + ExceptionDispatchInfo.Throw(ex); // Rethrow. + return; // Never reached. + case OperationCanceledException oce when oce.CancellationToken == cancellationToken: _stream.Abort(QuicAbortDirection.Read, (long)Http3ErrorCode.RequestCancelled); ExceptionDispatchInfo.Throw(ex); // Rethrow. diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 41543b32f73..dfe7f2e386c 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -36,7 +36,7 @@ private async Task AssertProtocolErrorAsync(Task task, ProtocolErrors errorCode) Assert.Equal(errorCode, (ProtocolErrors)protocolEx.ErrorCode); } - private async Task AssertProtocolErrorForIOExceptionAsync(Task task, ProtocolErrors errorCode) + private async Task AssertHttpProtocolException(Task task, ProtocolErrors errorCode) { HttpProtocolException protocolEx = await Assert.ThrowsAsync(() => task); Assert.Equal(errorCode, (ProtocolErrors)protocolEx.ErrorCode); @@ -2838,7 +2838,7 @@ public async Task PostAsyncDuplex_ServerResetsStream_Throws() await connection.WriteFrameAsync(new RstStreamFrame(FrameFlags.None, (int)ProtocolErrors.ENHANCE_YOUR_CALM, streamId)); // Trying to read on the response stream should fail now, and client should ignore any data received - await AssertProtocolErrorForIOExceptionAsync(SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId), ProtocolErrors.ENHANCE_YOUR_CALM); + await AssertHttpProtocolException(SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId), ProtocolErrors.ENHANCE_YOUR_CALM); // Attempting to write on the request body should now fail with IOException. Exception e = await Assert.ThrowsAnyAsync(async () => { await SendAndReceiveRequestDataAsync(contentBytes, requestStream, connection, streamId); }); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs index f322751bfdd..fa8eec76499 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs @@ -31,6 +31,15 @@ public HttpClientHandlerTest_Http3(ITestOutputHelper output) : base(output) { } + private async Task AssertProtocolErrorAsync(long errorCode, Func task) + { + Exception outerEx = await Assert.ThrowsAnyAsync(task); + _output.WriteLine(outerEx.ToString()); + Assert.IsType(outerEx); + HttpProtocolException protocolEx = Assert.IsType(outerEx.InnerException); + Assert.Equal(errorCode, protocolEx.ErrorCode); + } + [Theory] [InlineData(10)] // 2 bytes settings value. [InlineData(100)] // 4 bytes settings value. @@ -298,7 +307,117 @@ public async Task ReservedFrameType_Throws() VersionPolicy = HttpVersionPolicy.RequestVersionExact }; - await Assert.ThrowsAsync(async () => await client.SendAsync(request)); + await AssertProtocolErrorAsync(UnexpectedFrameErrorCode, () => client.SendAsync(request)); + }); + + await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); + } + + [Fact] + public async Task ServerClosesConnection_ThrowsHttpProtocolException() + { + const long GeneralProtocolError = 0x101; + + using Http3LoopbackServer server = CreateHttp3LoopbackServer(); + + Task serverTask = Task.Run(async () => + { + await using Http3LoopbackConnection connection = (Http3LoopbackConnection)await server.EstablishGenericConnectionAsync(); + await using Http3LoopbackStream stream = await connection.AcceptRequestStreamAsync(); + + await connection.CloseAsync(GeneralProtocolError); + }); + + Task clientTask = Task.Run(async () => + { + using HttpClient client = CreateHttpClient(); + using HttpRequestMessage request = new() + { + Method = HttpMethod.Get, + RequestUri = server.Address, + Version = HttpVersion30, + VersionPolicy = HttpVersionPolicy.RequestVersionExact + }; + + await AssertProtocolErrorAsync(GeneralProtocolError, () => client.SendAsync(request)); + }); + + await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); + } + + [Fact] + public async Task ServerClosesStream_ThrowsHttpProtocolException() + { + // normally, the server should not use this code when resetting the stream, but we should still check if we behave sanely... + const long GeneralProtocolError = 0x101; + + using Http3LoopbackServer server = CreateHttp3LoopbackServer(); + + SemaphoreSlim semaphore = new SemaphoreSlim(0); + Task serverTask = Task.Run(async () => + { + await using Http3LoopbackConnection connection = (Http3LoopbackConnection)await server.EstablishGenericConnectionAsync(); + await using Http3LoopbackStream stream = await connection.AcceptRequestStreamAsync(); + + stream.Abort(GeneralProtocolError); + await semaphore.WaitAsync(); + }); + + Task clientTask = Task.Run(async () => + { + using HttpClient client = CreateHttpClient(); + using HttpRequestMessage request = new() + { + Method = HttpMethod.Get, + RequestUri = server.Address, + Version = HttpVersion30, + VersionPolicy = HttpVersionPolicy.RequestVersionExact + }; + + await AssertProtocolErrorAsync(GeneralProtocolError, () => client.SendAsync(request)); + semaphore.Release(); + }); + + await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); + } + + [Fact] + public async Task ServerClosesConnection_ResponseContentStream_ThrowsHttpProtocolException() + { + const long GeneralProtocolError = 0x101; + + using Http3LoopbackServer server = CreateHttp3LoopbackServer(); + + SemaphoreSlim semaphore = new SemaphoreSlim(0); + Task serverTask = Task.Run(async () => + { + await using Http3LoopbackConnection connection = (Http3LoopbackConnection)await server.EstablishGenericConnectionAsync(); + await using Http3LoopbackStream stream = await connection.AcceptRequestStreamAsync(); + + await stream.ReadRequestBodyAsync(); + await stream.SendResponseHeadersAsync(); + await stream.SendDataFrameAsync(new byte[1024]); + await semaphore.WaitAsync(); + await connection.CloseAsync(GeneralProtocolError); + }); + + Task clientTask = Task.Run(async () => + { + using HttpClient client = CreateHttpClient(); + using HttpRequestMessage request = new() + { + Method = HttpMethod.Get, + RequestUri = server.Address, + Version = HttpVersion30, + VersionPolicy = HttpVersionPolicy.RequestVersionExact + }; + + var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); + var stream = await response.Content.ReadAsStreamAsync(); + await stream.ReadAsync(new byte[1024]); + semaphore.Release(); + var ex = await Assert.ThrowsAsync(async () => await stream.ReadAsync(new byte[1024])); + Assert.Equal(GeneralProtocolError, ex.ErrorCode); }); await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); diff --git a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs index 0f8a249a766..ab706fe1a0f 100644 --- a/src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs +++ b/src/libraries/System.Net.Http/tests/StressTests/HttpStress/ClientOperations.cs @@ -318,30 +318,11 @@ public static class ClientOperations } } - if (ctx.HttpVersion == HttpVersion.Version30) + if (ctx.HttpVersion == HttpVersion.Version30 && + e is HttpProtocolException protocolException && + protocolException.ErrorCode == 258) // 258 = H3_INTERNAL_ERROR (0x102) { - // HTTP/3 exception nesting: - // HttpRequestException->IOException->HttpRequestException->QuicStreamAbortedException - // HttpRequestException->QuicStreamAbortedException - - if (e is IOException && e.InnerException is HttpRequestException) - { - e = e.InnerException; - } - - if (e is HttpRequestException) - { - string? name = e.InnerException?.GetType().Name; - switch (name) - { - case "QuicStreamAbortedException": - if (e.InnerException?.Message?.Equals("Stream aborted by peer (258).") ?? false) // 258 = H3_INTERNAL_ERROR (0x102) - { - return; - } - break; - } - } + return; } throw; @@ -521,9 +502,9 @@ private static void ValidateContent(string expectedContent, string actualContent int divergentIndex = Enumerable .Zip(actualContent, expectedContent) - .Select((x,i) => (x.First, x.Second, i)) + .Select((x, i) => (x.First, x.Second, i)) .Where(x => x.First != x.Second) - .Select(x => (int?) x.i) + .Select(x => (int?)x.i) .FirstOrDefault() .GetValueOrDefault(Math.Min(actualContent.Length, expectedContent.Length)); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicException.cs index 40b8d38df25..f254c9b2f7a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicException.cs @@ -13,6 +13,9 @@ public sealed class QuicException : IOException /// /// Initializes a new instance of the class. /// + /// The error associated with the exception. + /// The application protocol error code associated with the error. + /// The message for the exception. public QuicException(QuicError error, long? applicationErrorCode, string message) : base(message) { -- GitLab