Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Apr 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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; }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) { }
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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())
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand All @@ -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
{
Expand Down Expand Up @@ -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));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}");
Expand Down Expand Up @@ -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() };
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
{
Expand Down
Loading