From: Krzysztof Wicher Date: Sat, 13 Jul 2019 04:07:20 +0000 (-0700) Subject: Retry on GOAWAY (dotnet/corefx#39127) X-Git-Tag: submit/tizen/20210909.063632~11031^2~951 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=03a36223e97c53b3d5dca67bd79de1e8bd88663d;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Retry on GOAWAY (dotnet/corefx#39127) * Retry on GOAWAY * send correct GOAWAY on retried connection, update comment * Harden against flakiness * Fix: Connection refused error is different on Debian * apply feedback * Make partial body/received headers case deterministic, pass error code on REFUSED_STREAM * apply latest feedback Commit migrated from https://github.com/dotnet/corefx/commit/1841042b99062de13dc80098cede9413be569238 --- diff --git a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs index 0982a61..3d20070 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs @@ -223,7 +223,7 @@ namespace System.Net.Test.Common // and ignore any meaningless frames -- i.e. WINDOW_UPDATE or expected SETTINGS ACK -- // that we see while waiting for the client to close. // Only call this after sending a GOAWAY. - public async Task WaitForConnectionShutdownAsync() + public async Task WaitForConnectionShutdownAsync(bool ignoreUnexpectedFrames = false) { // Shutdown our send side, so the client knows there won't be any more frames coming. ShutdownSend(); @@ -232,7 +232,10 @@ namespace System.Net.Test.Common Frame frame = await ReadFrameAsync(Timeout).ConfigureAwait(false); if (frame != null) { - throw new Exception($"Unexpected frame received while waiting for client shutdown: {frame}"); + if (!ignoreUnexpectedFrames) + { + throw new Exception($"Unexpected frame received while waiting for client shutdown: {frame}"); + } } _connectionStream.Close(); @@ -246,12 +249,12 @@ namespace System.Net.Test.Common // This is similar to WaitForConnectionShutdownAsync but will send GOAWAY for you // and will ignore any errors if client has already shutdown - public async Task ShutdownIgnoringErrorsAsync(int lastStreamId) + public async Task ShutdownIgnoringErrorsAsync(int lastStreamId, ProtocolErrors errorCode = ProtocolErrors.NO_ERROR) { try { - await SendGoAway(lastStreamId).ConfigureAwait(false); - await WaitForConnectionShutdownAsync().ConfigureAwait(false); + await SendGoAway(lastStreamId, errorCode).ConfigureAwait(false); + await WaitForConnectionShutdownAsync(ignoreUnexpectedFrames: true).ConfigureAwait(false); } catch (IOException) { @@ -276,6 +279,20 @@ namespace System.Net.Test.Common return frame.StreamId; } + public async Task ReadRequestHeaderFrameAsync() + { + // Receive HEADERS frame for request. + Frame frame = await ReadFrameAsync(Timeout).ConfigureAwait(false); + if (frame == null) + { + throw new IOException("Failed to read Headers frame."); + } + + Assert.Equal(FrameType.Headers, frame.Type); + Assert.Equal(FrameFlags.EndHeaders | FrameFlags.EndStream, frame.Flags); + return (HeadersFrame)frame; + } + private static (int bytesConsumed, int value) DecodeInteger(ReadOnlySpan headerBlock, byte prefixMask) { int value = headerBlock[0] & prefixMask; @@ -585,21 +602,24 @@ namespace System.Net.Test.Common return (streamId, requestData); } - public async Task SendGoAway(int lastStreamId) + public async Task SendGoAway(int lastStreamId, ProtocolErrors errorCode = ProtocolErrors.NO_ERROR) { - GoAwayFrame frame = new GoAwayFrame(lastStreamId, 0, new byte[] { }, 0); + GoAwayFrame frame = new GoAwayFrame(lastStreamId, (int)errorCode, new byte[] { }, 0); await WriteFrameAsync(frame).ConfigureAwait(false); } public async Task PingPong() { - PingFrame ping = new PingFrame(new byte[8] { 1, 2, 3, 4, 50, 60, 70, 80 }, FrameFlags.None, 0); + byte[] pingData = new byte[8] { 1, 2, 3, 4, 50, 60, 70, 80 }; + PingFrame ping = new PingFrame(pingData, FrameFlags.None, 0); await WriteFrameAsync(ping).ConfigureAwait(false); - Frame pingAck = await ReadFrameAsync(Timeout).ConfigureAwait(false); - if (pingAck.Type != FrameType.Ping || !pingAck.AckFlag) + PingFrame pingAck = (PingFrame)await ReadFrameAsync(Timeout).ConfigureAwait(false); + if (pingAck == null || pingAck.Type != FrameType.Ping || !pingAck.AckFlag) { throw new Exception("Expected PING ACK"); } + + Assert.Equal(pingData, pingAck.Data); } public async Task SendDefaultResponseHeadersAsync(int streamId) diff --git a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs index adc8cfe..d1642c5 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs @@ -219,4 +219,22 @@ namespace System.Net.Test.Common public override bool IsHttp11 => false; public override bool IsHttp2 => true; } + + public enum ProtocolErrors + { + NO_ERROR = 0x0, + PROTOCOL_ERROR = 0x1, + INTERNAL_ERROR = 0x2, + FLOW_CONTROL_ERROR = 0x3, + SETTINGS_TIMEOUT = 0x4, + STREAM_CLOSED = 0x5, + FRAME_SIZE_ERROR = 0x6, + REFUSED_STREAM = 0x7, + CANCEL = 0x8, + COMPRESSION_ERROR = 0x9, + CONNECT_ERROR = 0xa, + ENHANCE_YOUR_CALM = 0xb, + INADEQUATE_SECURITY = 0xc, + HTTP_1_1_REQUIRED = 0xd + } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestException.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestException.cs index d5f1e32..a826bc7 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestException.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestException.cs @@ -31,7 +31,7 @@ namespace System.Net.Http // This constructor is used internally to indicate that a request was not successfully sent due to an IOException, // and the exception occurred early enough so that the request may be retried on another connection. - internal HttpRequestException(string message, IOException inner, bool allowRetry) + internal HttpRequestException(string message, Exception inner, bool allowRetry) : this(message, inner) { AllowRetry = allowRetry; 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 c436745..e8a764b 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 @@ -659,7 +659,7 @@ namespace System.Net.Http if (protocolError == Http2ProtocolErrorCode.RefusedStream) { - http2Stream.OnRefused(); + http2Stream.OnAbort(new Http2StreamException(protocolError), canRetry: true); } else { @@ -689,7 +689,7 @@ namespace System.Net.Http var errorCode = (Http2ProtocolErrorCode)BinaryPrimitives.ReadInt32BigEndian(_incomingBuffer.ActiveSpan.Slice(sizeof(int))); if (NetEventSource.IsEnabled) Trace(frameHeader.StreamId, $"{nameof(lastValidStream)}={lastValidStream}, {nameof(errorCode)}={errorCode}"); - AbortStreams(lastValidStream, new Http2ConnectionException(errorCode)); + StartTerminatingConnection(lastValidStream, new Http2ConnectionException(errorCode)); _incomingBuffer.Discard(frameHeader.Length); } @@ -1278,7 +1278,7 @@ namespace System.Net.Http { // The connection has failed, e.g. failed IO or a connection-level frame error. Interlocked.CompareExchange(ref _abortException, abortException, null); - AbortStreams(0, abortException); + AbortStreams(abortException); } /// Gets whether the connection exceeded any of the connection limits. @@ -1316,10 +1316,40 @@ namespace System.Net.Http return LifetimeExpired(nowTicks, connectionLifetime); } - private void AbortStreams(int lastValidStream, Exception abortException) + private void AbortStreams(Exception abortException) + { + // Invalidate outside of lock to avoid race with HttpPool Dispose() + // We should not try to grab pool lock while holding connection lock as on disposing pool, + // we could hold pool lock while trying to grab connection lock in Dispose(). + _pool.InvalidateHttp2Connection(this); + + lock (SyncObject) + { + if (NetEventSource.IsEnabled) Trace($"{nameof(abortException)}={abortException}"); + + foreach (KeyValuePair kvp in _httpStreams) + { + int streamId = kvp.Key; + Debug.Assert(streamId == kvp.Value.StreamId); + + kvp.Value.OnAbort(abortException); + } + + _httpStreams.Clear(); + + _disposed = true; + CheckForShutdown(); + } + } + + private void StartTerminatingConnection(int lastValidStream, Exception abortException) { Debug.Assert(lastValidStream >= 0); - bool isAlreadyInvalidated = _disposed; + + // Invalidate outside of lock to avoid race with HttpPool Dispose() + // We should not try to grab pool lock while holding connection lock as on disposing pool, + // we could hold pool lock while trying to grab connection lock in Dispose(). + _pool.InvalidateHttp2Connection(this); lock (SyncObject) { @@ -1332,27 +1362,25 @@ namespace System.Net.Http // We have already received GOAWAY before // In this case the smaller valid stream is used _lastValidStreamId = Math.Min(_lastValidStreamId, lastValidStream); - isAlreadyInvalidated = true; } - if (NetEventSource.IsEnabled) Trace($"{nameof(lastValidStream)}={lastValidStream}, {nameof(abortException)}={lastValidStream}={abortException}, {nameof(_lastValidStreamId)}={_lastValidStreamId}"); + if (NetEventSource.IsEnabled) Trace($"{nameof(lastValidStream)}={lastValidStream}, {nameof(_lastValidStreamId)}={_lastValidStreamId}"); bool hasAnyActiveStream = false; + foreach (KeyValuePair kvp in _httpStreams) { int streamId = kvp.Key; Debug.Assert(streamId == kvp.Value.StreamId); - if (streamId > lastValidStream) + if (streamId > _lastValidStreamId) { - kvp.Value.OnAbort(abortException); - - _httpStreams.Remove(kvp.Value.StreamId); + kvp.Value.OnAbort(abortException, canRetry: true); + _httpStreams.Remove(streamId); } else { - if (NetEventSource.IsEnabled) Trace($"Found {nameof(streamId)} {streamId} <= {lastValidStream}."); - + if (NetEventSource.IsEnabled) Trace($"Found {nameof(streamId)} {streamId} <= {_lastValidStreamId}."); hasAnyActiveStream = true; } } @@ -1364,14 +1392,6 @@ namespace System.Net.Http CheckForShutdown(); } - - if (!isAlreadyInvalidated) - { - // Invalidate outside of lock to avoid race with HttpPool Dispose() - // We should not try to grab pool lock while holding connection lock as on disposing pool, - // we could hold pool lock while trying to grab connection lock in Dispose(). - _pool.InvalidateHttp2Connection(this); - } } private void CheckForShutdown() 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 6be326d..c24cd09 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 @@ -445,7 +445,7 @@ namespace System.Net.Http } } - public void OnAbort(Exception abortException) + public void OnAbort(Exception abortException, bool canRetry = false) { bool signalWaiter; lock (SyncObject) @@ -457,33 +457,13 @@ namespace System.Net.Http return; } + // We should not retry request which have started being processed since the behavior might be unpredictable + // I.e. some action might have been taken based on the received data. + // We will bubble the exception and let user decide what to do. + bool isRetriable = _state == StreamState.ExpectingStatus || _state == StreamState.ExpectingHeaders; 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; - } - - _state = StreamState.Aborted; - _canRetry = true; + _canRetry = canRetry && isRetriable; signalWaiter = _hasWaiter; _hasWaiter = false; @@ -508,7 +488,7 @@ namespace System.Net.Http { if (_canRetry) { - throw CreateRetryException(); + throw new HttpRequestException(SR.net_http_request_aborted, _abortException, allowRetry: true); } throw new IOException(SR.net_http_request_aborted, _abortException); 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 a055d0cd..40f2fef 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -9,12 +10,12 @@ using System.Net.Security; using System.Linq; using System.Net.Test.Common; using System.Text; +using System.Text.Unicode; using System.Threading; using System.Threading.Tasks; using Xunit; using Xunit.Abstractions; -using System.Collections; namespace System.Net.Http.Functional.Tests { @@ -40,22 +41,24 @@ namespace System.Net.Http.Functional.Tests } } - public enum ProtocolErrors + private async Task<(bool, T)> IgnoreSpecificException(Task task, string expectedExceptionContent = null) where ExpectedException : Exception { - NO_ERROR = 0x0, - PROTOCOL_ERROR = 0x1, - INTERNAL_ERROR = 0x2, - FLOW_CONTROL_ERROR = 0x3, - SETTINGS_TIMEOUT = 0x4, - STREAM_CLOSED = 0x5, - FRAME_SIZE_ERROR = 0x6, - REFUSED_STREAM = 0x7, - CANCEL = 0x8, - COMPRESSION_ERROR = 0x9, - CONNECT_ERROR = 0xa, - ENHANCE_YOUR_CALM = 0xb, - INADEQUATE_SECURITY = 0xc, - HTTP_1_1_REQUIRED = 0xd + try + { + return (true, await task); + } + catch (ExpectedException e) + { + if (expectedExceptionContent != null) + { + if (!e.ToString().Contains(expectedExceptionContent)) + { + throw; + } + } + + return (false, default(T)); + } } [Fact] @@ -689,12 +692,12 @@ namespace System.Net.Http.Functional.Tests } [ConditionalFact(nameof(SupportsAlpn))] - [ActiveIssue(39013)] public async Task GoAwayFrame_UnprocessedStreamFirstRequestFinishedFirst_RequestRestarted() { // This test case is similar to GoAwayFrame_UnprocessedStreamFirstRequestWaitsUntilSecondFinishes_RequestRestarted // but is easier: we close first connection before we expect retry to happen + using (await Watchdog.CreateAsync()) using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) { @@ -734,9 +737,9 @@ namespace System.Net.Http.Functional.Tests } [ConditionalFact(nameof(SupportsAlpn))] - [ActiveIssue(39013)] public async Task GoAwayFrame_UnprocessedStreamFirstRequestWaitsUntilSecondFinishes_RequestRestarted() { + using (await Watchdog.CreateAsync()) using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) { @@ -988,7 +991,7 @@ namespace System.Net.Http.Functional.Tests [ConditionalFact(nameof(SupportsAlpn))] public async Task GoAwayFrame_NoPendingStreams_ConnectionClosed() { - using (new Timer(s => Console.WriteLine(GetStateMachineData.Describe(s)), await GetStateMachineData.FetchAsync(), 60_000, 60_000)) + using (await Watchdog.CreateAsync()) using (var server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) { @@ -1073,52 +1076,66 @@ namespace System.Net.Http.Functional.Tests [ConditionalFact(nameof(SupportsAlpn))] public async Task GoAwayFrame_AbortAllPendingStreams_StreamFailWithExpectedException() { - using (new Timer(s => Console.WriteLine(GetStateMachineData.Describe(s)), await GetStateMachineData.FetchAsync(), 60_000, 60_000)) + using (await Watchdog.CreateAsync()) using (Http2LoopbackServer server = Http2LoopbackServer.CreateServer()) using (HttpClient client = CreateHttpClient()) { - (_, Http2LoopbackConnection connection) = await EstablishConnectionAndProcessOneRequestAsync(client, server); + client.BaseAddress = server.Address; + server.AllowMultipleConnections = true; - // Issue three requests - Task sendTask1 = client.GetAsync(server.Address); - Task sendTask2 = client.GetAsync(server.Address); - Task sendTask3 = client.GetAsync(server.Address); + (_, Http2LoopbackConnection connection) = await EstablishConnectionAndProcessOneRequestAsync(client, server); - // Receive three requests + // Issue three requests, we want to make sure the specific task is related with specific stream + Task sendTask1 = client.GetAsync("request1"); int streamId1 = await connection.ReadRequestHeaderAsync(); + + Task sendTask2 = client.GetAsync("request2"); int streamId2 = await connection.ReadRequestHeaderAsync(); + + Task sendTask3 = client.GetAsync("request3"); int streamId3 = await connection.ReadRequestHeaderAsync(); - Assert.InRange(streamId1, int.MinValue, streamId2 - 1); - Assert.InRange(streamId2, int.MinValue, streamId3 - 1); + Assert.InRange(streamId1, 1, streamId2 - 1); + Assert.InRange(streamId2, streamId1 + 1, streamId3 - 1); + Assert.InRange(streamId3, streamId2 + 1, Int32.MaxValue); // Send various partial responses - // First response: Don't send anything yet + // First response: Don't send anything yet, request should be retried on the new connection - // Second response: Send headers, no body yet + // Second response: Send headers, no body yet - request should fail await connection.SendDefaultResponseHeadersAsync(streamId2); - // Third response: Send headers, partial body + // Third response: Send headers, partial body - request should fail await connection.SendDefaultResponseHeadersAsync(streamId3); await connection.SendResponseDataAsync(streamId3, new byte[5], endStream: false); - // Send a GOAWAY frame that indicates that we will abort all the requests. - var goAwayFrame = new GoAwayFrame(0, (int)ProtocolErrors.ENHANCE_YOUR_CALM, new byte[0], 0); - await connection.WriteFrameAsync(goAwayFrame); + // Ensure all sent frames are received by client + await connection.PingPong(); - // We will not send any more frames, so send EOF now, and ensure the client handles this properly. - connection.ShutdownSend(); + // Send a GOAWAY frame that indicates that we have not processed any of the requests + await connection.SendGoAway(0, ProtocolErrors.ENHANCE_YOUR_CALM); + + Http2LoopbackConnection newConnection = await server.EstablishConnectionAsync(); + + HeadersFrame retriedFrame = await newConnection.ReadRequestHeaderFrameAsync(); + int retriedStreamId = retriedFrame.StreamId; + Assert.InRange(retriedStreamId, 1, Int32.MaxValue); + string headerData = Encoding.UTF8.GetString(retriedFrame.Data.Span); + + await newConnection.SendDefaultResponseHeadersAsync(retriedStreamId); + await newConnection.SendResponseDataAsync(retriedStreamId, new byte[3], endStream: true); + + Assert.Contains("request1", headerData); + + HttpResponseMessage response1 = await sendTask1; + Assert.Equal(HttpStatusCode.OK, response1.StatusCode); + await newConnection.ShutdownIgnoringErrorsAsync(retriedStreamId, ProtocolErrors.ENHANCE_YOUR_CALM); - await AssertProtocolErrorAsync(sendTask1, ProtocolErrors.ENHANCE_YOUR_CALM); await AssertProtocolErrorAsync(sendTask2, ProtocolErrors.ENHANCE_YOUR_CALM); await AssertProtocolErrorAsync(sendTask3, ProtocolErrors.ENHANCE_YOUR_CALM); - // Now that all pending responses have been sent, the client should close the connection. await connection.WaitForConnectionShutdownAsync(); - - // New request should cause a new connection - await EstablishConnectionAndProcessOneRequestAsync(client, server); } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj index 2b38963..bc19dce 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj @@ -140,6 +140,7 @@ + diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/Watchdog.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/Watchdog.cs new file mode 100644 index 0000000..21758e9 --- /dev/null +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/Watchdog.cs @@ -0,0 +1,69 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace System.Net.Http.Functional.Tests +{ + /// + /// This is using similar trick to GetStateMachineData + /// If marked test runs for more than 60s it will print machine state and make sure it fails + /// Usage (await MUST be run directly in the test, should not be called from other async method): + /// using (await Watchdog.CreateAsync()) + /// { + /// // test code + /// } + /// + internal class Watchdog : ICriticalNotifyCompletion + { + private object _box; + + private Watchdog() { } + + public static Watchdog CreateAsync() + => new Watchdog(); + + public IDisposable GetResult() + => new WatchdogImpl(_box); + + public Watchdog GetAwaiter() => this; + public bool IsCompleted => false; + public void OnCompleted(Action continuation) => UnsafeOnCompleted(continuation); + public void UnsafeOnCompleted(Action continuation) + { + _box = continuation.Target; + Task.Run(continuation); + } + + private class WatchdogImpl : IDisposable + { + private bool _passed = true; + private Timer _timer; + + public WatchdogImpl(object stateMachineData) + { + _timer = new Timer(s => + { + _passed = false; + Console.WriteLine(GetStateMachineData.Describe(s)); + }, + stateMachineData, + 60_000, + 60_000); + } + + public void Dispose() + { + _timer.Dispose(); + Assert.True(_passed); + } + } + } +}