From b64f89fc8cce3235d5b27eda9ac41360e76d1ec6 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 08:48:25 +0200 Subject: [PATCH 1/6] Throw HttpProtocolException in case we get a GOAWAY frame while waiting for next frame on response --- .../SocketsHttpHandler/Http2Connection.cs | 25 +++++++------- .../HttpClientHandlerTest.Http2.cs | 33 +++++++++++++++++++ 2 files changed, 45 insertions(+), 13 deletions(-) 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 40202ff846d4a..37e3c018d03c9 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 @@ -72,6 +72,8 @@ internal sealed partial class Http2Connection : HttpConnectionBase // _shutdown above is true, and requests in flight have been (or are being) failed. private Exception? _abortException; + private Http2ProtocolErrorCode? _goAwayErrorCode; + private const int MaxStreamId = int.MaxValue; // Temporary workaround for request burst handling on connection start. @@ -410,14 +412,10 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) _incomingBuffer.Commit(bytesRead); if (bytesRead == 0) { - if (_incomingBuffer.ActiveLength == 0) - { - ThrowMissingFrame(); - } - else - { - ThrowPrematureEOF(FrameHeader.Size); - } + HttpIOException exception = _incomingBuffer.ActiveLength == 0 ? + ThrowMissingFrame() : ThrowPrematureEOF(FrameHeader.Size); + throw _goAwayErrorCode is not null ? + new HttpProtocolException((long)_goAwayErrorCode, exception.Message, exception) : exception; } } while (_incomingBuffer.ActiveLength < FrameHeader.Size); @@ -449,7 +447,7 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) int bytesRead = await _stream.ReadAsync(_incomingBuffer.AvailableMemory).ConfigureAwait(false); _incomingBuffer.Commit(bytesRead); - if (bytesRead == 0) ThrowPrematureEOF(frameHeader.PayloadLength); + if (bytesRead == 0) throw ThrowPrematureEOF(frameHeader.PayloadLength); } while (_incomingBuffer.ActiveLength < frameHeader.PayloadLength); } @@ -457,11 +455,11 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) // Return the read frame header. return frameHeader; - void ThrowPrematureEOF(int requiredBytes) => - throw new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); + HttpIOException ThrowPrematureEOF(int requiredBytes) => + new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); - void ThrowMissingFrame() => - throw new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); + HttpIOException ThrowMissingFrame() => + new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); } private async Task ProcessIncomingFramesAsync() @@ -1070,6 +1068,7 @@ private void ProcessGoAwayFrame(FrameHeader frameHeader) Debug.Assert(lastStreamId >= 0); Exception resetException = HttpProtocolException.CreateHttp2ConnectionException(errorCode, SR.net_http_http2_connection_close); + _goAwayErrorCode = errorCode; // There is no point sending more PING frames for RTT estimation: _rttEstimator.OnGoAwayReceived(); 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 256a04e62da26..7e067be1d357a 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -1051,6 +1051,39 @@ public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndTh } } + [ConditionalFact(nameof(SupportsAlpn))] + public async Task GoAwayFrame_RequestServerDisconnects_ThrowsHttpProtocolExceptionWithProperErrorCode() + { + _output.WriteLine("Started!"); + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + // Client starts an HTTP/2 request and awaits response headers + using HttpClient client = CreateHttpClient(); + HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); + _output.WriteLine("After response header read"); + // Client reads from response stream + using Stream responseStream = await response.Content.ReadAsStreamAsync(); + Memory buffer = new byte[1024]; + HttpProtocolException exception = await Assert.ThrowsAsync(() => responseStream.ReadAsync(buffer).AsTask()); + Assert.Equal(ProtocolErrors.ENHANCE_YOUR_CALM, (ProtocolErrors) exception.ErrorCode); + Assert.IsType(exception.InnerException); + Assert.Contains("ended prematurely while waiting", exception.Message); + + }, + async server => + { + // Server returns response headers + Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); + int streamId = await connection.ReadRequestHeaderAsync(); + await connection.SendDefaultResponseHeadersAsync(streamId); + _output.WriteLine("Server sent response headers!"); + // Server sends GOAWAY frame + await connection.SendGoAway(streamId, ProtocolErrors.ENHANCE_YOUR_CALM); + _output.WriteLine("Server sent GOAWAY!"); + connection.ShutdownSend(); + }).WaitAsync(TimeSpan.FromSeconds(10)); + } + [ConditionalFact(nameof(SupportsAlpn))] public async Task GoAwayFrame_UnprocessedStreamFirstRequestFinishedFirst_RequestRestarted() { From c975b900416f3cdfc9bede71d56389f8effdbef2 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 09:05:20 +0200 Subject: [PATCH 2/6] Fix helper method names --- .../System/Net/Http/SocketsHttpHandler/Http2Connection.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 37e3c018d03c9..dd81cfbd754d9 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 @@ -413,7 +413,7 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) if (bytesRead == 0) { HttpIOException exception = _incomingBuffer.ActiveLength == 0 ? - ThrowMissingFrame() : ThrowPrematureEOF(FrameHeader.Size); + GetMissingFrameException() : GetPrematureEOFException(FrameHeader.Size); throw _goAwayErrorCode is not null ? new HttpProtocolException((long)_goAwayErrorCode, exception.Message, exception) : exception; } @@ -447,7 +447,7 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) int bytesRead = await _stream.ReadAsync(_incomingBuffer.AvailableMemory).ConfigureAwait(false); _incomingBuffer.Commit(bytesRead); - if (bytesRead == 0) throw ThrowPrematureEOF(frameHeader.PayloadLength); + if (bytesRead == 0) throw GetPrematureEOFException(frameHeader.PayloadLength); } while (_incomingBuffer.ActiveLength < frameHeader.PayloadLength); } @@ -455,10 +455,10 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) // Return the read frame header. return frameHeader; - HttpIOException ThrowPrematureEOF(int requiredBytes) => + HttpIOException GetPrematureEOFException(int requiredBytes) => new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); - HttpIOException ThrowMissingFrame() => + HttpIOException GetMissingFrameException() => new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); } From c1e6015eab67d09ae0075875172611d5223fe446 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 15:42:53 +0200 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Miha Zupan --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 7e067be1d357a..4e8c3817f4045 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -1054,7 +1054,6 @@ public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndTh [ConditionalFact(nameof(SupportsAlpn))] public async Task GoAwayFrame_RequestServerDisconnects_ThrowsHttpProtocolExceptionWithProperErrorCode() { - _output.WriteLine("Started!"); await Http2LoopbackServer.CreateClientAndServerAsync(async uri => { // Client starts an HTTP/2 request and awaits response headers @@ -1073,7 +1072,7 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => async server => { // Server returns response headers - Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); + await using Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); int streamId = await connection.ReadRequestHeaderAsync(); await connection.SendDefaultResponseHeadersAsync(streamId); _output.WriteLine("Server sent response headers!"); @@ -1081,7 +1080,7 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => await connection.SendGoAway(streamId, ProtocolErrors.ENHANCE_YOUR_CALM); _output.WriteLine("Server sent GOAWAY!"); connection.ShutdownSend(); - }).WaitAsync(TimeSpan.FromSeconds(10)); + }); } [ConditionalFact(nameof(SupportsAlpn))] From d4cb83e155c9b6de31bf59d4a875ded6100038bc Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 16:02:19 +0200 Subject: [PATCH 4/6] Code review feedback --- .../SocketsHttpHandler/Http2Connection.cs | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) 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 dd81cfbd754d9..fa148a36b1b7e 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 @@ -412,10 +412,18 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) _incomingBuffer.Commit(bytesRead); if (bytesRead == 0) { - HttpIOException exception = _incomingBuffer.ActiveLength == 0 ? - GetMissingFrameException() : GetPrematureEOFException(FrameHeader.Size); - throw _goAwayErrorCode is not null ? - new HttpProtocolException((long)_goAwayErrorCode, exception.Message, exception) : exception; + if (_goAwayErrorCode is not null) + { + ThrowProtocolError(_goAwayErrorCode.Value, SR.net_http_http2_connection_close); + } + else if (_incomingBuffer.ActiveLength == 0) + { + ThrowMissingFrameException(); + } + else + { + ThrowPrematureEOFException(FrameHeader.Size); + } } } while (_incomingBuffer.ActiveLength < FrameHeader.Size); @@ -447,7 +455,7 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) int bytesRead = await _stream.ReadAsync(_incomingBuffer.AvailableMemory).ConfigureAwait(false); _incomingBuffer.Commit(bytesRead); - if (bytesRead == 0) throw GetPrematureEOFException(frameHeader.PayloadLength); + if (bytesRead == 0) ThrowPrematureEOFException(frameHeader.PayloadLength); } while (_incomingBuffer.ActiveLength < frameHeader.PayloadLength); } @@ -455,11 +463,11 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) // Return the read frame header. return frameHeader; - HttpIOException GetPrematureEOFException(int requiredBytes) => - new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); + void ThrowPrematureEOFException(int requiredBytes) => + throw new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); - HttpIOException GetMissingFrameException() => - new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); + void ThrowMissingFrameException() => + throw new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); } private async Task ProcessIncomingFramesAsync() From 2bb76ec45465b74b9abc8ce227243e49e08674c8 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 16:04:27 +0200 Subject: [PATCH 5/6] Revert method names --- .../Net/Http/SocketsHttpHandler/Http2Connection.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 fa148a36b1b7e..8b2c37122e56a 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 @@ -418,11 +418,11 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) } else if (_incomingBuffer.ActiveLength == 0) { - ThrowMissingFrameException(); + ThrowMissingFrame(); } else { - ThrowPrematureEOFException(FrameHeader.Size); + ThrowPrematureEOF(FrameHeader.Size); } } } @@ -455,7 +455,7 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) int bytesRead = await _stream.ReadAsync(_incomingBuffer.AvailableMemory).ConfigureAwait(false); _incomingBuffer.Commit(bytesRead); - if (bytesRead == 0) ThrowPrematureEOFException(frameHeader.PayloadLength); + if (bytesRead == 0) ThrowPrematureEOF(frameHeader.PayloadLength); } while (_incomingBuffer.ActiveLength < frameHeader.PayloadLength); } @@ -463,10 +463,10 @@ private async ValueTask ReadFrameAsync(bool initialFrame = false) // Return the read frame header. return frameHeader; - void ThrowPrematureEOFException(int requiredBytes) => + void ThrowPrematureEOF(int requiredBytes) => throw new HttpIOException(HttpRequestError.ResponseEnded, SR.Format(SR.net_http_invalid_response_premature_eof_bytecount, requiredBytes - _incomingBuffer.ActiveLength)); - void ThrowMissingFrameException() => + void ThrowMissingFrame() => throw new HttpIOException(HttpRequestError.ResponseEnded, SR.net_http_invalid_response_missing_frame); } From a07ecc5c18cc97a553eeea60e9d743234c5d5b48 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 11 Jul 2024 17:33:03 +0200 Subject: [PATCH 6/6] Fix test with the new behavior --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 4e8c3817f4045..1a24fe7e0832a 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -1059,14 +1059,13 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => // Client starts an HTTP/2 request and awaits response headers using HttpClient client = CreateHttpClient(); HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); - _output.WriteLine("After response header read"); + // Client reads from response stream using Stream responseStream = await response.Content.ReadAsStreamAsync(); Memory buffer = new byte[1024]; HttpProtocolException exception = await Assert.ThrowsAsync(() => responseStream.ReadAsync(buffer).AsTask()); Assert.Equal(ProtocolErrors.ENHANCE_YOUR_CALM, (ProtocolErrors) exception.ErrorCode); - Assert.IsType(exception.InnerException); - Assert.Contains("ended prematurely while waiting", exception.Message); + Assert.Contains("The HTTP/2 server closed the connection.", exception.Message); }, async server => @@ -1075,10 +1074,9 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => await using Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); int streamId = await connection.ReadRequestHeaderAsync(); await connection.SendDefaultResponseHeadersAsync(streamId); - _output.WriteLine("Server sent response headers!"); + // Server sends GOAWAY frame await connection.SendGoAway(streamId, ProtocolErrors.ENHANCE_YOUR_CALM); - _output.WriteLine("Server sent GOAWAY!"); connection.ShutdownSend(); }); }