From d5757c16bd66ec798308a6e9d7d07517a681cab4 Mon Sep 17 00:00:00 2001 From: Geoff Kizer Date: Fri, 28 Jun 2019 17:42:27 -0700 Subject: [PATCH] enable request retry when receiving REFUSED_STREAM protocol error Commit migrated from https://github.com/dotnet/corefx/commit/b251108b7fd0b3601eb6f8561ec11fd966671a0e --- .../Net/Http/SocketsHttpHandler/Http2Connection.cs | 11 +++- .../Net/Http/SocketsHttpHandler/Http2Stream.cs | 74 +++++++++++++++------ .../FunctionalTests/HttpClientHandlerTest.Http2.cs | 75 ++++++++++++++++++++++ 3 files changed, 137 insertions(+), 23 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 d46f4b6..f66f480 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 @@ -643,7 +643,14 @@ namespace System.Net.Http _incomingBuffer.Discard(frameHeader.Length); - http2Stream.OnResponseAbort(new Http2ProtocolException(protocolError)); + if (protocolError == Http2ProtocolErrorCode.RefusedStream) + { + http2Stream.OnRefused(); + } + else + { + http2Stream.OnAbort(new Http2ProtocolException(protocolError)); + } RemoveStream(http2Stream); } @@ -1289,7 +1296,7 @@ namespace System.Net.Http if (streamId > lastValidStream) { - kvp.Value.OnResponseAbort(abortException); + kvp.Value.OnAbort(abortException); _httpStreams.Remove(kvp.Value.StreamId); } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index 05469c7..3ed5dad 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -39,6 +39,7 @@ namespace System.Net.Http private StreamState _state; private bool _disposed; private Exception _abortException; + private bool _canRetry; // if _state == Aborted, this indicates the stream was refused and so the request is retryable /// /// The core logic for the IValueTaskSource implementation. @@ -93,6 +94,8 @@ namespace System.Net.Http _headerBudgetRemaining = connection._pool.Settings._maxResponseHeadersLength * 1024; + _canRetry = false; + if (NetEventSource.IsEnabled) Trace($"{request}, {nameof(initialWindowSize)}={initialWindowSize}"); } @@ -447,25 +450,45 @@ namespace System.Net.Http } } - public void OnResponseAbort(Exception abortException) + public void OnAbort(Exception abortException) { bool signalWaiter; lock (SyncObject) { if (NetEventSource.IsEnabled) Trace($"{nameof(abortException)}={abortException}"); - if (_disposed) + if (_disposed || _state == StreamState.Aborted) { return; } - if (_state == StreamState.Aborted) + Interlocked.CompareExchange(ref _abortException, abortException, null); + _state = StreamState.Aborted; + + signalWaiter = _hasWaiter; + _hasWaiter = false; + } + + if (signalWaiter) + { + _waitSource.SetResult(true); + } + } + + public void OnRefused() + { + bool signalWaiter; + lock (SyncObject) + { + if (NetEventSource.IsEnabled) Trace(""); + + if (_disposed || _state == StreamState.Aborted) { return; } - Interlocked.CompareExchange(ref _abortException, abortException, null); _state = StreamState.Aborted; + _canRetry = true; signalWaiter = _hasWaiter; _hasWaiter = false; @@ -477,21 +500,37 @@ namespace System.Net.Http } } + private void CheckIfDisposedOrAborted() + { + Debug.Assert(Monitor.IsEntered(SyncObject)); + + if (_disposed) + { + throw new ObjectDisposedException(nameof(Http2Stream)); + } + + if (_state == StreamState.Aborted) + { + if (_canRetry) + { + // Throw a retryable request exception. + // This will cause retry logic to kick in and perform another connection attempt. + // The user should never see this exception. + throw new HttpRequestException(null, null, allowRetry: true); + } + + throw new IOException(SR.net_http_request_aborted, _abortException); + } + } + // Determine if we have enough data to process up to complete final response headers. private (bool wait, bool isEmptyResponse) TryEnsureHeaders() { lock (SyncObject) { - if (_disposed) - { - throw new ObjectDisposedException(nameof(Http2Stream)); - } + CheckIfDisposedOrAborted(); - if (_state == StreamState.Aborted) - { - throw new IOException(SR.net_http_request_aborted, _abortException); - } - else if (_state == StreamState.ExpectingHeaders || _state == StreamState.ExpectingIgnoredHeaders || _state == StreamState.ExpectingStatus) + if (_state == StreamState.ExpectingHeaders || _state == StreamState.ExpectingIgnoredHeaders || _state == StreamState.ExpectingStatus) { Debug.Assert(!_hasWaiter); _hasWaiter = true; @@ -568,10 +607,7 @@ namespace System.Net.Http lock (SyncObject) { - if (_disposed) - { - throw new ObjectDisposedException(nameof(Http2Stream)); - } + CheckIfDisposedOrAborted(); if (_responseBuffer.ActiveSpan.Length > 0) { @@ -585,10 +621,6 @@ namespace System.Net.Http { return (false, 0); } - else if (_state == StreamState.Aborted) - { - throw new IOException(SR.net_http_request_aborted, _abortException); - } Debug.Assert(_state == StreamState.ExpectingData || _state == StreamState.ExpectingTrailingHeaders); 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 7fa0b98..c1eda8d 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -294,6 +294,80 @@ namespace System.Net.Http.Functional.Tests } } + [ConditionalFact(nameof(SupportsAlpn))] + public async Task GetAsync_StreamRefused_RequestIsRetried() + { + using (var server = Http2LoopbackServer.CreateServer()) + using (HttpClient client = CreateHttpClient()) + { + (_, Http2LoopbackConnection connection) = await EstablishConnectionAndProcessOneRequestAsync(client, server); + + Task sendTask = client.GetAsync(server.Address); + (int streamId1, HttpRequestData requestData1) = await connection.ReadAndParseRequestHeaderAsync(readBody: true); + + // Send SETTINGS frame to change stream limit to 0, allowing us to send a REFUSED_STREAM error due to the new limit + connection.ExpectSettingsAck(); + Frame frame = new SettingsFrame(new SettingsEntry() { SettingId = SettingId.MaxConcurrentStreams, Value = 0 }); + await connection.WriteFrameAsync(frame); + + // Send a RST_STREAM with error = REFUSED_STREAM, indicating the request can be retried. + frame = new RstStreamFrame(FrameFlags.None, (int)ProtocolErrors.REFUSED_STREAM, streamId1); + await connection.WriteFrameAsync(frame); + + await connection.PingPong(); + + // Send SETTINGS frame to change stream limit to 1, allowing the request to now be processed + connection.ExpectSettingsAck(); + frame = new SettingsFrame(new SettingsEntry() { SettingId = SettingId.MaxConcurrentStreams, Value = 1 }); + await connection.WriteFrameAsync(frame); + + // Process retried request + (int streamId2, HttpRequestData requestData2) = await connection.ReadAndParseRequestHeaderAsync(readBody: true); + await connection.SendDefaultResponseAsync(streamId2); + HttpResponseMessage response = await sendTask; + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + } + + [ConditionalFact(nameof(SupportsAlpn))] + public async Task PostAsync_StreamRefused_RequestIsRetried() + { + using (var server = Http2LoopbackServer.CreateServer()) + using (HttpClient client = CreateHttpClient()) + { + const string Content = "Hello world"; + + (_, Http2LoopbackConnection connection) = await EstablishConnectionAndProcessOneRequestAsync(client, server); + + Task sendTask = client.PostAsync(server.Address, new StringContent(Content)); + (int streamId1, HttpRequestData requestData1) = await connection.ReadAndParseRequestHeaderAsync(readBody: true); + Assert.Equal(Content, Encoding.UTF8.GetString(requestData1.Body)); + + // Send SETTINGS frame to change stream limit to 0, allowing us to send a REFUSED_STREAM error due to the new limit + connection.ExpectSettingsAck(); + Frame frame = new SettingsFrame(new SettingsEntry() { SettingId = SettingId.MaxConcurrentStreams, Value = 0 }); + await connection.WriteFrameAsync(frame); + + // Send a RST_STREAM with error = REFUSED_STREAM, indicating the request can be retried. + frame = new RstStreamFrame(FrameFlags.None, (int)ProtocolErrors.REFUSED_STREAM, streamId1); + await connection.WriteFrameAsync(frame); + + await connection.PingPong(); + + // Send SETTINGS frame to change stream limit to 1, allowing the request to now be processed + connection.ExpectSettingsAck(); + frame = new SettingsFrame(new SettingsEntry() { SettingId = SettingId.MaxConcurrentStreams, Value = 1 }); + await connection.WriteFrameAsync(frame); + + // Process retried request + (int streamId2, HttpRequestData requestData2) = await connection.ReadAndParseRequestHeaderAsync(readBody: true); + Assert.Equal(Content, Encoding.UTF8.GetString(requestData2.Body)); + await connection.SendDefaultResponseAsync(streamId2); + HttpResponseMessage response = await sendTask; + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + } + // This test is based on RFC 7540 section 6.1: // "If a DATA frame is received whose stream identifier field is 0x0, the recipient MUST // respond with a connection error (Section 5.4.1) of type PROTOCOL_ERROR." @@ -720,6 +794,7 @@ namespace System.Net.Http.Functional.Tests public static IEnumerable ValidAndInvalidProtocolErrors() => Enum.GetValues(typeof(ProtocolErrors)) .Cast() + .Where(e => e != ProtocolErrors.REFUSED_STREAM) // REFUSED_STREAM allows retry instead of abort, so skip it .Concat(new[] { (ProtocolErrors)12345 }) .Select(p => new object[] { p }); -- 2.7.4