From 08e00bea231ad8e5afa83fc9d4e74ed667f16997 Mon Sep 17 00:00:00 2001 From: Geoff Kizer Date: Mon, 26 Apr 2021 18:37:54 -0700 Subject: [PATCH] rework HTTP version handling (#51454) * rework HTTP version handling * address review feedback * fix version error string Co-authored-by: Geoffrey Kizer --- .../src/Resources/Strings.resx | 3 - .../SocketsHttpHandler/HttpConnectionPool.cs | 349 +++++++++--------- .../SocketsHttpHandler/SocketsHttpHandler.cs | 9 + 3 files changed, 182 insertions(+), 179 deletions(-) diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index 897207df43ddc..109e4adcecd7c 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -579,9 +579,6 @@ HTTP request version upgrade is not enabled for synchronous '{0}'. Do not use '{1}' version policy for synchronous HTTP methods. - - Requesting HTTP version {0} with version policy {1} while HTTP/{2} is not enabled. - Requesting HTTP version {0} with version policy {1} while unable to establish HTTP/{2} connection. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs index 5d2d98ff86889..af64342469513 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs @@ -370,100 +370,35 @@ public byte[] Http2AltSvcOriginUri /// Object used to synchronize access to state in the pool. private object SyncObj => _idleConnections; - private ValueTask GetConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) + private static HttpRequestException GetVersionException(HttpRequestMessage request, int desiredVersion) { - // Do not even attempt at getting/creating a connection if it's already obvious we cannot provided the one requested. - if (request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) - { - if (request.Version.Major == 3 && !_http3Enabled) - { - return ValueTask.FromException( - new HttpRequestException(SR.Format(SR.net_http_requested_version_not_enabled, request.Version, request.VersionPolicy, 3))); - } - if (request.Version.Major == 2 && !_http2Enabled) - { - return ValueTask.FromException( - new HttpRequestException(SR.Format(SR.net_http_requested_version_not_enabled, request.Version, request.VersionPolicy, 2))); - } - } - - // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished - if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) - { - // Either H3 explicitly requested or secured upgraded allowed. - if (_http3Enabled && (request.Version.Major >= 3 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure))) - { - HttpAuthority? authority = _http3Authority; - // H3 is explicitly requested, assume prenegotiated H3. - if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) - { - authority = authority ?? _originAuthority; - } - if (authority != null) - { - if (IsAltSvcBlocked(authority)) - { - return ValueTask.FromException( - new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 3))); - } - - return GetHttp3ConnectionAsync(request, authority, cancellationToken); - } - } - } + Debug.Assert(desiredVersion == 2 || desiredVersion == 3); - // If we got here, we cannot provide HTTP/3 connection. Do not continue if downgrade is not allowed. - if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) - { - return ValueTask.FromException( - new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 3))); - } - - if (_http2Enabled && (request.Version.Major >= 2 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure)) && - // If the connection is not secured and downgrade is possible, prefer HTTP/1.1. - (request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower || IsSecure)) - { - return GetHttp2ConnectionAsync(request, async, cancellationToken); - } - // If we got here, we cannot provide HTTP/2 connection. Do not continue if downgrade is not allowed. - if (request.Version.Major >= 2 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) - { - return ValueTask.FromException( - new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 2))); - } - - return GetHttp11ConnectionAsync(request, async, cancellationToken); + return new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, desiredVersion)); } private ValueTask GetOrReserveHttp11ConnectionAsync(bool async, CancellationToken cancellationToken) { - if (cancellationToken.IsCancellationRequested) - { - return ValueTask.FromCanceled(cancellationToken); - } - - List idleConnections = _idleConnections; long nowTicks = Environment.TickCount64; // Look for a usable idle connection. - // If we can't find one, we will either wait for one to become available (if at the connection limit) - // or just increment the connection count and return null so the caller can create a new connection. TaskCompletionSourceWithCancellation waiter; while (true) { HttpConnection connection; lock (SyncObj) { - if (idleConnections.Count > 0) + int idleConnectionCount = _idleConnections.Count; + if (idleConnectionCount > 0) { - // We have a cached connection that we can attempt to use. - // Test it below outside the lock, to avoid doing expensive validation while holding the lock. - connection = idleConnections[idleConnections.Count - 1]._connection; - idleConnections.RemoveAt(idleConnections.Count - 1); + // We have an idle connection that we can attempt to use. + // Validate it below outside the lock, to avoid doing expensive operations while holding the lock. + connection = _idleConnections[idleConnectionCount - 1]._connection; + _idleConnections.RemoveAt(idleConnectionCount - 1); } else { - // No valid cached connections. + // No available idle connections. if (_associatedConnectionCount < _maxConnections) { // We are under the connection limit, so just increment the count and return null @@ -533,28 +468,8 @@ private ValueTask GetConnectionAsync(HttpRequestMessage requ } } - private async ValueTask GetHttp11ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) - { - HttpConnection? connection = await GetOrReserveHttp11ConnectionAsync(async, cancellationToken).ConfigureAwait(false); - if (connection is not null) - { - return connection; - } - - if (NetEventSource.Log.IsEnabled()) Trace("Creating new HTTP/1.1 connection for pool."); - - try - { - return await CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false); - } - catch - { - DecrementConnectionCount(); - throw; - } - } - - private async ValueTask GetHttp2ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) + // Returns null if HTTP2 cannot be used + private async ValueTask GetHttp2ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) { Debug.Assert(_kind == HttpConnectionKind.Https || _kind == HttpConnectionKind.SslProxyTunnel || _kind == HttpConnectionKind.Http || _kind == HttpConnectionKind.SocksTunnel || _kind == HttpConnectionKind.SslSocksTunnel); @@ -655,7 +570,8 @@ private async ValueTask GetHttp2ConnectionAsync(HttpRequestM if (sslStream != null) { // We established an SSL connection, but the server denied our request for HTTP2. - // Continue as an HTTP/1.1 connection. + // Try to establish a 1.1 connection instead and add it to the pool. + if (NetEventSource.Log.IsEnabled()) { Trace("Server does not support HTTP2; disabling HTTP2 use and proceeding with HTTP/1.1 connection"); @@ -664,34 +580,25 @@ private async ValueTask GetHttp2ConnectionAsync(HttpRequestM bool canUse = true; lock (SyncObj) { + // Server does not support HTTP2. Disable further HTTP2 attempts. _http2Enabled = false; - if (request.Version.Major >= 2 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) - { - sslStream.Close(); - throw new HttpRequestException(SR.Format(SR.net_http_requested_version_server_refused, request.Version, request.VersionPolicy)); - } - if (_associatedConnectionCount < _maxConnections) { IncrementConnectionCountNoLock(); } else { - // We are in the weird situation of having established a new HTTP 1.1 connection - // when we were already at the maximum for HTTP 1.1 connections. - // Just discard this connection and get another one from the pool. - // This should be a really rare situation to get into, since it would require - // the user to make multiple HTTP 1.1-only requests first before attempting an - // HTTP2 request, and the server failing to accept HTTP2. + // We are already at the limit for HTTP/1.1 connections, so do not proceed with this connection. canUse = false; } } if (canUse) { - return await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false); - } + HttpConnection http11Connection = await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false); + ReturnConnection(http11Connection); + } else { if (NetEventSource.Log.IsEnabled()) @@ -703,8 +610,8 @@ private async ValueTask GetHttp2ConnectionAsync(HttpRequestM } } - // If we reach this point, it means we need to fall back to a (new or existing) HTTP/1.1 connection. - return await GetHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false); + // We are unable to use HTTP2. + return null; } private Http2Connection? GetExistingHttp2Connection() @@ -759,7 +666,7 @@ private void AddHttp2Connection(Http2Connection newConnection) [SupportedOSPlatform("windows")] [SupportedOSPlatform("linux")] [SupportedOSPlatform("macos")] - private async ValueTask GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) + private async ValueTask GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { Debug.Assert(_kind == HttpConnectionKind.Https); Debug.Assert(_http3Enabled == true); @@ -854,38 +761,162 @@ private async ValueTask GetHttp3ConnectionAsync(HttpRequestM } } - public async ValueTask SendWithRetryAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + // Returns null if HTTP3 cannot be used. + // TODO: SupportedOSPlatform doesn't work for internal APIs https://github.com/dotnet/runtime/issues/51305 + [SupportedOSPlatform("windows")] + [SupportedOSPlatform("linux")] + [SupportedOSPlatform("macos")] + private async ValueTask TrySendUsingHttp3Async(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) { - int retryCount = 0; - - while (true) + if (_http3Enabled && (request.Version.Major >= 3 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure))) { - // Loop on connection failures (or other problems like version downgrade) and retry if possible. - - HttpConnectionBase connection = await GetConnectionAsync(request, async, cancellationToken).ConfigureAwait(false); + // Loop in case we get a 421 and need to send the request to a different authority. + while (true) + { + HttpAuthority? authority = _http3Authority; - HttpResponseMessage response; + // If H3 is explicitly requested, assume prenegotiated H3. + if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) + { + authority = authority ?? _originAuthority; + } - try - { - if (connection is HttpConnection) + if (authority != null) { - ((HttpConnection)connection).Acquire(); - try + if (IsAltSvcBlocked(authority)) { - response = await (doRequestAuth && Settings._credentials != null ? - AuthenticationHelper.SendWithNtConnectionAuthAsync(request, async, Settings._credentials, (HttpConnection)connection, this, cancellationToken) : - SendWithNtProxyAuthAsync((HttpConnection)connection, request, async, cancellationToken)).ConfigureAwait(false); + throw GetVersionException(request, 3); } - finally + + Http3Connection connection = await GetHttp3ConnectionAsync(request, authority, cancellationToken).ConfigureAwait(false); + HttpResponseMessage response = await connection.SendAsync(request, async, cancellationToken).ConfigureAwait(false); + + // If an Alt-Svc authority returns 421, it means it can't actually handle the request. + // An authority is supposed to be able to handle ALL requests to the origin, so this is a server bug. + // In this case, we blocklist the authority and retry the request at the origin. + if (response.StatusCode == HttpStatusCode.MisdirectedRequest && connection.Authority != _originAuthority) { - ((HttpConnection)connection).Release(); + response.Dispose(); + BlocklistAuthority(connection.Authority); + continue; } + + return response; } - else - { - response = await connection!.SendAsync(request, async, cancellationToken).ConfigureAwait(false); - } + } + } + + return null; + } + + // Returns null if HTTP2 cannot be used. + private async ValueTask TrySendUsingHttp2Async(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + { + // Send using HTTP/2 if we can. + if (_http2Enabled && (request.Version.Major >= 2 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure)) && + // If the connection is not secured and downgrade is possible, prefer HTTP/1.1. + (request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower || IsSecure)) + { + Http2Connection? connection = await GetHttp2ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false); + if (connection is null) + { + Debug.Assert(!_http2Enabled); + return null; + } + + return await connection.SendAsync(request, async, cancellationToken).ConfigureAwait(false); + } + + return null; + } + + private async ValueTask SendUsingHttp11Async(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + { + HttpConnection? connection = await GetOrReserveHttp11ConnectionAsync(async, cancellationToken).ConfigureAwait(false); + if (connection is null) + { + if (NetEventSource.Log.IsEnabled()) Trace("Creating new HTTP/1.1 connection for pool."); + + try + { + connection = await CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false); + } + catch + { + DecrementConnectionCount(); + throw; + } + } + + // In case we are doing Windows (i.e. connection-based) auth, we need to ensure that we hold on to this specific connection while auth is underway. + connection.Acquire(); + try + { + return await SendWithNtConnectionAuthAsync(connection, request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); + } + finally + { + connection.Release(); + } + } + + private async ValueTask DetermineVersionAndSendAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + { + HttpResponseMessage? response; + + // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished + if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) + { + response = await TrySendUsingHttp3Async(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); + if (response is not null) + { + return response; + } + } + + // We cannot use HTTP/3. Do not continue if downgrade is not allowed. + if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) + { + throw GetVersionException(request, 3); + } + + response = await TrySendUsingHttp2Async(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); + if (response is not null) + { + return response; + } + + // We cannot use HTTP/2. Do not continue if downgrade is not allowed. + if (request.Version.Major >= 2 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) + { + throw GetVersionException(request, 2); + } + + return await SendUsingHttp11Async(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); + } + + private async ValueTask SendAndProcessAltSvcAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + { + HttpResponseMessage response = await DetermineVersionAndSendAsync(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); + + // Check for the Alt-Svc header, to upgrade to HTTP/3. + if (_altSvcEnabled && response.Headers.TryGetValues(KnownHeaders.AltSvc.Descriptor, out IEnumerable? altSvcHeaderValues)) + { + HandleAltSvc(altSvcHeaderValues, response.Headers.Age); + } + + return response; + } + + public async ValueTask SendWithRetryAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + { + int retryCount = 0; + while (true) + { + // Loop on connection failures (or other problems like version downgrade) and retry if possible. + try + { + return await SendAndProcessAltSvcAsync(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false); } catch (HttpRequestException e) when (e.AllowRetry == RequestRetryType.RetryOnConnectionFailure) { @@ -909,7 +940,6 @@ public async ValueTask SendWithRetryAsync(HttpRequestMessag } // Eat exception and try again. - continue; } catch (HttpRequestException e) when (e.AllowRetry == RequestRetryType.RetryOnLowerHttpVersion) { @@ -925,9 +955,7 @@ public async ValueTask SendWithRetryAsync(HttpRequestMessag } // Eat exception and try again on a lower protocol version. - Debug.Assert(connection is HttpConnection == false, $"{nameof(RequestRetryType.RetryOnLowerHttpVersion)} should not be thrown by HTTP/1 connections."); request.Version = HttpVersion.Version11; - continue; } catch (HttpRequestException e) when (e.AllowRetry == RequestRetryType.RetryOnStreamLimitReached) { @@ -937,30 +965,7 @@ public async ValueTask SendWithRetryAsync(HttpRequestMessag } // Eat exception and try again. - continue; - } - - // Check for the Alt-Svc header, to upgrade to HTTP/3. - if (_altSvcEnabled && response.Headers.TryGetValues(KnownHeaders.AltSvc.Descriptor, out IEnumerable? altSvcHeaderValues)) - { - HandleAltSvc(altSvcHeaderValues, response.Headers.Age); - } - - // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished - if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) - { - // If an Alt-Svc authority returns 421, it means it can't actually handle the request. - // An authority is supposed to be able to handle ALL requests to the origin, so this is a server bug. - // In this case, we blocklist the authority and retry the request at the origin. - if (response.StatusCode == HttpStatusCode.MisdirectedRequest && connection is Http3Connection h3Connection && h3Connection.Authority != _originAuthority) - { - response.Dispose(); - BlocklistAuthority(h3Connection.Authority); - continue; - } } - - return response; } } @@ -1190,22 +1195,14 @@ public void OnNetworkChanged() } } - public async Task SendWithNtConnectionAuthAsync(HttpConnection connection, HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) + public Task SendWithNtConnectionAuthAsync(HttpConnection connection, HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) { - connection.Acquire(); - try - { - if (doRequestAuth && Settings._credentials != null) - { - return await AuthenticationHelper.SendWithNtConnectionAuthAsync(request, async, Settings._credentials, connection, this, cancellationToken).ConfigureAwait(false); - } - - return await SendWithNtProxyAuthAsync(connection, request, async, cancellationToken).ConfigureAwait(false); - } - finally + if (doRequestAuth && Settings._credentials != null) { - connection.Release(); + return AuthenticationHelper.SendWithNtConnectionAuthAsync(request, async, Settings._credentials, connection, this, cancellationToken); } + + return SendWithNtProxyAuthAsync(connection, request, async, cancellationToken); } private bool DoProxyAuth => (_kind == HttpConnectionKind.Proxy || _kind == HttpConnectionKind.ProxyConnect); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 003a33a8c66c6..8e0b508e6a485 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -511,6 +511,9 @@ protected internal override HttpResponseMessage Send(HttpRequestMessage request, } CheckDisposed(); + + cancellationToken.ThrowIfCancellationRequested(); + HttpMessageHandlerStage handler = _handler ?? SetupHandlerChain(); Exception? error = ValidateAndNormalizeRequest(request); @@ -525,6 +528,12 @@ protected internal override HttpResponseMessage Send(HttpRequestMessage request, protected internal override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { CheckDisposed(); + + if (cancellationToken.IsCancellationRequested) + { + return Task.FromCanceled(cancellationToken); + } + HttpMessageHandler handler = _handler ?? SetupHandlerChain(); Exception? error = ValidateAndNormalizeRequest(request);