Rework request retry logic to be based on retry count limit (#48758)

* rework request retry logic to be based off a fixed retry limit (MaxConnectionFailureRetries) instead of isNewConnection logic

* be more conservative about request retries -- only allow on receiving EOF or GOAWAY from the server


Co-authored-by: Geoffrey Kizer <geoffrek@windows.microsoft.com>

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

@@ -91,7 +91,15 @@ private async Task ReadPrefixAsync()
 
             if (Text.Encoding.ASCII.GetString(_prefix).Contains("HTTP/1.1"))
             {
-                throw new Exception("HTTP 1.1 request received.");
+                // Tests that use HttpAgnosticLoopbackServer will attempt to send an HTTP/1.1 request to an HTTP/2 server.
+                // This is invalid and we should terminate the connection.
+                // However, if we simply terminate the connection without sending anything, then this could be interpreted
+                // as a server disconnect that should be retried by SocketsHttpHandler.
+                // Since those tests are not set up to handle multiple retries, we instead just send back an invalid response here
+                // so that SocketsHttpHandler will not induce retry.
+                // The contents of what we send don't really matter, as long as it is interpreted by SocketsHttpHandler as an invalid response.
+                await _connectionStream.WriteAsync(Encoding.ASCII.GetBytes("HTTP/2.0 400 Bad Request\r\n\r\n"));
+                _connectionSocket.Shutdown(SocketShutdown.Send);
             }
         }
 

src/libraries/Common/tests/System/Net/Http/HttpAgnosticLoopbackServer.cs

@@ -5,14 +5,10 @@
 using System.IO;
 using System.Net.Security;
 using System.Net.Sockets;
-using System.Security.Authentication;
 using System.Security.Cryptography.X509Certificates;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 
-using Xunit;
-
 namespace System.Net.Test.Common
 {
     public class HttpAgnosticLoopbackServer : GenericLoopbackServer, IDisposable
@@ -105,43 +101,18 @@ public override async Task<GenericLoopbackConnection> EstablishGenericConnection
                     }
                 }
 
-                if (_options.ClearTextVersion is null)
+                if (_options.ClearTextVersion == HttpVersion.Version11)
                 {
-                    throw new Exception($"HTTP server does not accept clear text connections, either set '{nameof(HttpAgnosticOptions.UseSsl)}' or set up '{nameof(HttpAgnosticOptions.ClearTextVersion)}' in server options.");
+                    return connection = await Http11LoopbackServerFactory.Singleton.CreateConnectionAsync(socket, stream, options).ConfigureAwait(false);
                 }
-
-                var buffer = new byte[24];
-                var position = 0;
-                while (position < buffer.Length)
-                {
-                    var readBytes = await stream.ReadAsync(buffer, position, buffer.Length - position).ConfigureAwait(false);
-                    if (readBytes == 0)
-                    {
-                        break;
-                    }
-                    position += readBytes;
-                }
-
-                var memory = new Memory<byte>(buffer, 0, position);
-                stream = new ReturnBufferStream(stream, memory);
-
-                var prefix = Text.Encoding.ASCII.GetString(memory.Span);
-                if (prefix == Http2LoopbackConnection.Http2Prefix)
+                else if (_options.ClearTextVersion == HttpVersion.Version20)
                 {
-                    if (_options.ClearTextVersion == HttpVersion.Version20 || _options.ClearTextVersion == HttpVersion.Unknown)
-                    {
-                        return connection = await Http2LoopbackServerFactory.Singleton.CreateConnectionAsync(socket, stream, options).ConfigureAwait(false);
-                    }
+                    return connection = await Http2LoopbackServerFactory.Singleton.CreateConnectionAsync(socket, stream, options).ConfigureAwait(false);
                 }
-                else
+                else 
                 {
-                    if (_options.ClearTextVersion == HttpVersion.Version11 || _options.ClearTextVersion == HttpVersion.Unknown)
-                    {
-                        return connection = await Http11LoopbackServerFactory.Singleton.CreateConnectionAsync(socket, stream, options).ConfigureAwait(false);
-                    }
+                    throw new Exception($"Invalid ClearTextVersion={_options.ClearTextVersion} specified");
                 }
-
-                throw new Exception($"HTTP/{_options.ClearTextVersion} server cannot establish connection due to unexpected data: '{prefix}'");
             }
             catch
             {            
@@ -194,8 +165,7 @@ public static async Task CreateClientAndServerAsync(Func<Uri, Task> clientFunc,
 
     public class HttpAgnosticOptions : GenericLoopbackOptions
     {
-        // Default null will raise an exception for any clear text protocol version
-        // Use HttpVersion.Unknown to use protocol version detection for clear text.
+        // Must specify either HttpVersion.Version11 or HttpVersion.Version20.
         public Version ClearTextVersion { get; set; }
         public List<SslApplicationProtocol> SslApplicationProtocols { get; set; }
     }

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs

@@ -161,11 +161,6 @@ public PlatformHandler_IdnaProtocolTests(ITestOutputHelper output) : base(output
         protected override bool SupportsIdna => !PlatformDetection.IsWindows7;
     }
 
-    public sealed class PlatformHandler_HttpRetryProtocolTests : HttpRetryProtocolTests
-    {
-        public PlatformHandler_HttpRetryProtocolTests(ITestOutputHelper output) : base(output) { }
-    }
-
     public sealed class PlatformHandlerTest_Cookies : HttpClientHandlerTest_Cookies
     {
         public PlatformHandlerTest_Cookies(ITestOutputHelper output) : base(output) { }
@@ -340,13 +335,6 @@ public PlatformHandler_IdnaProtocol_Http2_Tests(ITestOutputHelper output) : base
         protected override bool SupportsIdna => !PlatformDetection.IsWindows7;
     }
 
-    public sealed class PlatformHandler_HttpRetryProtocol_Http2_Tests : HttpRetryProtocolTests
-    {
-        protected override Version UseVersion => HttpVersion20.Value;
-
-        public PlatformHandler_HttpRetryProtocol_Http2_Tests(ITestOutputHelper output) : base(output) { }
-    }
-
     public sealed class PlatformHandlerTest_Cookies_Http11_Http2 : HttpClientHandlerTest_Cookies_Http11
     {
         protected override Version UseVersion => HttpVersion20.Value;

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj

@@ -109,8 +109,6 @@
              Link="Common\System\Net\Http\HttpClient.SelectedSitesTest.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\HttpClientEKUTest.cs"
              Link="Common\System\Net\Http\HttpClientEKUTest.cs" />
-    <Compile Include="$(CommonTestPath)System\Net\Http\HttpRetryProtocolTests.cs"
-             Link="Common\System\Net\Http\HttpRetryProtocolTests.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\IdnaProtocolTests.cs"
              Link="Common\System\Net\Http\IdnaProtocolTests.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\LoopbackProxyServer.cs"

src/libraries/System.Net.Http/src/System/Net/Http/RequestRetryType.cs

@@ -14,22 +14,24 @@ internal enum RequestRetryType
         NoRetry,
 
         /// <summary>
-        /// The request failed on the current HTTP version, and the server requested it be retried on a lower version.
+        /// The request failed due to e.g. server shutting down (GOAWAY) and should be retried on a new connection.
         /// </summary>
-        RetryOnLowerHttpVersion,
+        RetryOnConnectionFailure,
 
         /// <summary>
-        /// The request failed due to e.g. server shutting down (GOAWAY) and should be retried on a new connection.
+        /// The request failed on the current HTTP version, and the server requested it be retried on a lower version.
         /// </summary>
-        RetryOnSameOrNextProxy,
+        RetryOnLowerHttpVersion,
 
         /// <summary>
         /// The proxy failed, so the request should be retried on the next proxy.
         /// </summary>
         RetryOnNextProxy,
 
+        /// <summary>
         /// The HTTP/2 connection reached the maximum number of streams and
         /// another HTTP/2 connection must be created or found to serve the request.
-        RetryOnNextConnection
+        /// </summary>
+        RetryOnStreamLimitReached
     }
 }

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

@@ -1271,24 +1271,22 @@ private void ThrowShutdownException()
         {
             Debug.Assert(Monitor.IsEntered(SyncObject));
 
-            // Throw a retryable exception that will allow this unprocessed request to be processed on a new connection.
-            // In rare cases, such as receiving GOAWAY immediately after connection establishment, we will not
-            // actually retry the request, so we must give a useful exception here for these cases.
-
-            Exception innerException;
             if (_abortException != null)
             {
-                innerException = _abortException;
+                // We had an IO failure on the connection. Don't retry in this case.
+                throw new HttpRequestException(SR.net_http_client_execution_error, _abortException);
             }
-            else if (_lastStreamId != -1)
+
+            // Connection is being gracefully shutdown. Allow the request to be retried.
+            Exception innerException;
+            if (_lastStreamId != -1)
             {
                 // We must have received a GOAWAY.
                 innerException = new IOException(SR.net_http_server_shutdown);
             }
             else
             {
                 // We must either be disposed or out of stream IDs.
-                // Note that in this case, the exception should never be visible to the user (it should be retried).
                 Debug.Assert(_disposed || _nextStream == MaxStreamId);
 
                 innerException = new ObjectDisposedException(nameof(Http2Connection));
@@ -1307,7 +1305,7 @@ private async ValueTask<Http2Stream> SendHeadersAsync(HttpRequestMessage request
                 {
                     if (_pool.EnableMultipleHttp2Connections)
                     {
-                        throw new HttpRequestException(null, null, RequestRetryType.RetryOnNextConnection);
+                        throw new HttpRequestException(null, null, RequestRetryType.RetryOnStreamLimitReached);
                     }
 
                     if (HttpTelemetry.Log.IsEnabled())
@@ -2014,7 +2012,7 @@ internal void Trace(int streamId, string message, [CallerMemberName] string? mem
 
         [DoesNotReturn]
         private static void ThrowRetry(string message, Exception innerException) =>
-            throw new HttpRequestException(message, innerException, allowRetry: RequestRetryType.RetryOnSameOrNextProxy);
+            throw new HttpRequestException(message, innerException, allowRetry: RequestRetryType.RetryOnConnectionFailure);
 
         private static Exception GetRequestAbortedException(Exception? innerException = null) =>
             new IOException(SR.net_http_request_aborted, innerException);

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs

@@ -209,7 +209,7 @@ public override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage req
 
                 if (quicStream == null)
                 {
-                    throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnSameOrNextProxy);
+                    throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
                 }
 
                 // 0-byte write to force QUIC to allocate a stream ID.
@@ -224,7 +224,7 @@ public override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage req
 
                 if (goAway)
                 {
-                    throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnSameOrNextProxy);
+                    throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
                 }
 
                 Task<HttpResponseMessage> responseTask = requestStream.SendAsync(cancellationToken);
@@ -238,7 +238,7 @@ public override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage req
             {
                 // This will happen if we aborted _connection somewhere.
                 Abort(ex);
-                throw new HttpRequestException(SR.Format(SR.net_http_http3_connection_error, ex.ErrorCode), ex, RequestRetryType.RetryOnSameOrNextProxy);
+                throw new HttpRequestException(SR.Format(SR.net_http_http3_connection_error, ex.ErrorCode), ex, RequestRetryType.RetryOnConnectionFailure);
             }
             finally
             {
@@ -281,7 +281,7 @@ private void CancelWaiters()
 
             while (_waitingRequests.TryDequeue(out TaskCompletionSourceWithCancellation<bool>? tcs))
             {
-                tcs.TrySetException(new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnSameOrNextProxy));
+                tcs.TrySetException(new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure));
             }
         }
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs

@@ -269,7 +269,7 @@ public async Task<HttpResponseMessage> SendAsync(CancellationToken cancellationT
                 else
                 {
                     Debug.Assert(_goawayCancellationToken.IsCancellationRequested == true);
-                    throw new HttpRequestException(SR.net_http_request_aborted, ex, RequestRetryType.RetryOnSameOrNextProxy);
+                    throw new HttpRequestException(SR.net_http_request_aborted, ex, RequestRetryType.RetryOnConnectionFailure);
                 }
             }
             catch (Http3ConnectionException ex)

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -63,6 +63,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase, IDisposable
 
         private bool _inUse;
         private bool _canRetry;
+        private bool _startedSendingRequestBody;
         private bool _connectionClose; // Connection: close was seen on last response
 
         private const int Status_Disposed = 1;
@@ -385,8 +386,8 @@ public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request,
             _currentRequest = request;
             HttpMethod normalizedMethod = HttpMethod.Normalize(request.Method);
 
-            Debug.Assert(!_canRetry);
-            _canRetry = true;
+            _canRetry = false;
+            _startedSendingRequestBody = false;
 
             // Send the request.
             if (NetEventSource.Log.IsEnabled()) Trace($"Sending request: {request}");
@@ -561,19 +562,28 @@ public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request,
 
                     if (NetEventSource.Log.IsEnabled()) Trace($"Received {bytesRead} bytes.");
 
-                    if (bytesRead == 0)
+                    _readOffset = 0;
+                    _readLength = bytesRead;
+                }
+                else
+                {
+                    // No read-ahead, so issue a read ourselves. We will check below for EOF.
+                    await InitialFillAsync(async).ConfigureAwait(false);
+                }
+
+                if (_readLength == 0)
+                {
+                    // The server shutdown the connection on their end, likely because of an idle timeout.
+                    // If we haven't started sending the request body yet (or there is no request body),
+                    // then we allow the request to be retried.
+                    if (!_startedSendingRequestBody)
                     {
-                        throw new IOException(SR.net_http_invalid_response_premature_eof);
+                        _canRetry = true;
                     }
 
-                    _readOffset = 0;
-                    _readLength = bytesRead;
+                    throw new IOException(SR.net_http_invalid_response_premature_eof);
                 }
 
-                // The request is no longer retryable; either we received data from the _readAheadTask,
-                // or there was no _readAheadTask because this is the first request on the connection.
-                // (We may have already set this as well if we sent request content.)
-                _canRetry = false;
 
                 // Parse the response status line.
                 var response = new HttpResponseMessage() { RequestMessage = request, Content = new HttpConnectionResponseContent() };
@@ -821,7 +831,7 @@ private bool MapSendException(Exception exception, CancellationToken cancellatio
             {
                 // For consistency with other handlers we wrap the exception in an HttpRequestException.
                 // If the request is retryable, indicate that on the exception.
-                mappedException = new HttpRequestException(SR.net_http_client_execution_error, ioe, _canRetry ? RequestRetryType.RetryOnSameOrNextProxy : RequestRetryType.NoRetry);
+                mappedException = new HttpRequestException(SR.net_http_client_execution_error, ioe, _canRetry ? RequestRetryType.RetryOnConnectionFailure : RequestRetryType.NoRetry);
                 return true;
             }
             // Otherwise, just allow the original exception to propagate.
@@ -863,8 +873,8 @@ private CancellationTokenRegistration RegisterCancellation(CancellationToken can
 
         private async ValueTask SendRequestContentAsync(HttpRequestMessage request, HttpContentWriteStream stream, bool async, CancellationToken cancellationToken)
         {
-            // Now that we're sending content, prohibit retries on this connection.
-            _canRetry = false;
+            // Now that we're sending content, prohibit retries of this request by setting this flag.
+            _startedSendingRequestBody = true;
 
             Debug.Assert(stream.BytesWritten == 0);
             if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestContentStart();
@@ -1550,6 +1560,19 @@ private void Fill()
             fillTask.GetAwaiter().GetResult();
         }
 
+        // Does not throw on EOF. Also assumes there is no buffered data.
+        private async ValueTask InitialFillAsync(bool async)
+        {
+            Debug.Assert(_readAheadTask == null);
+
+            _readOffset = 0;
+            _readLength = async ?
+                await _stream.ReadAsync(_readBuffer).ConfigureAwait(false) :
+                _stream.Read(_readBuffer);
+
+            if (NetEventSource.Log.IsEnabled()) Trace($"Received {_readLength} bytes.");
+        }
+
         // Throws IOException on EOF.  This is only called when we expect more data.
         private async ValueTask FillAsync(bool async)
         {