From 1e478ebe0249927cc5945145d6cf81c88e4de43b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 10 Mar 2021 23:07:40 -0500 Subject: [PATCH 1/2] Fix polling on https connections in HttpConnectionPool Avoid performing read-aheads on such connections, which can result in long-pinned buffers and unnecessary faulted tasks when the connections are torn down. --- .../Http/SocketsHttpHandler/HttpConnection.cs | 6 +-- .../SocketsHttpHandler/HttpConnectionPool.cs | 46 +++++++++++++------ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index 485b600e9c5a0..9d5aa5dac2d97 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -71,6 +71,7 @@ internal partial class HttpConnection : HttpConnectionBase, IDisposable public HttpConnection( HttpConnectionPool pool, + Socket? socket, Stream stream, TransportContext? transportContext) { @@ -79,10 +80,7 @@ public HttpConnection( _pool = pool; _stream = stream; - if (stream is NetworkStream networkStream) - { - _socket = networkStream.Socket; - } + _socket = socket; _transportContext = transportContext; 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 c60e5ca8a35f8..6184dd0327383 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 @@ -567,6 +567,7 @@ private static bool IsUsableHttp11Connection(HttpConnection connection, long now } // Try to establish an HTTP2 connection + Socket? socket = null; Stream? stream = null; SslStream? sslStream = null; TransportContext? transportContext = null; @@ -591,7 +592,7 @@ private static bool IsUsableHttp11Connection(HttpConnection connection, long now HttpResponseMessage? failureResponse; - (stream, transportContext, failureResponse) = + (socket, stream, transportContext, failureResponse) = await ConnectAsync(request, async, cancellationToken).ConfigureAwait(false); if (failureResponse != null) @@ -681,7 +682,7 @@ private static bool IsUsableHttp11Connection(HttpConnection connection, long now if (canUse) { - return (await ConstructHttp11ConnectionAsync(async, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), true, null); + return (await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), true, null); } else { @@ -1213,7 +1214,7 @@ public ValueTask SendAsync(HttpRequestMessage request, bool return SendWithProxyAuthAsync(request, async, doRequestAuth, cancellationToken); } - private async ValueTask<(Stream?, TransportContext?, HttpResponseMessage?)> ConnectAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) + private async ValueTask<(Socket?, Stream?, TransportContext?, HttpResponseMessage?)> ConnectAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) { // If a non-infinite connect timeout has been set, create and use a new CancellationToken that will be canceled // when either the original token is canceled or a connect timeout occurs. @@ -1228,17 +1229,18 @@ public ValueTask SendAsync(HttpRequestMessage request, bool try { Stream? stream = null; + Socket? socket = null; switch (_kind) { case HttpConnectionKind.Http: case HttpConnectionKind.Https: case HttpConnectionKind.ProxyConnect: Debug.Assert(_originAuthority != null); - stream = await ConnectToTcpHostAsync(_originAuthority.IdnHost, _originAuthority.Port, request, async, cancellationToken).ConfigureAwait(false); + (socket, stream) = await ConnectToTcpHostAsync(_originAuthority.IdnHost, _originAuthority.Port, request, async, cancellationToken).ConfigureAwait(false); break; case HttpConnectionKind.Proxy: - stream = await ConnectToTcpHostAsync(_proxyUri!.IdnHost, _proxyUri.Port, request, async, cancellationToken).ConfigureAwait(false); + (socket, stream) = await ConnectToTcpHostAsync(_proxyUri!.IdnHost, _proxyUri.Port, request, async, cancellationToken).ConfigureAwait(false); break; case HttpConnectionKind.ProxyTunnel: @@ -1249,12 +1251,18 @@ public ValueTask SendAsync(HttpRequestMessage request, bool { // Return non-success response from proxy. response.RequestMessage = request; - return (null, null, response); + return (null, null, null, response); } break; } Debug.Assert(stream != null); + if (socket is null && stream is NetworkStream ns) + { + // We weren't handed a socket directly. But if we're able to extract one, do so. + // Most likely case here is a ConnectCallback was used and returned a NetworkStream. + socket = ns.Socket; + } TransportContext? transportContext = null; if (IsSecure) @@ -1264,7 +1272,7 @@ public ValueTask SendAsync(HttpRequestMessage request, bool stream = sslStream; } - return (stream, transportContext, null); + return (socket, stream, transportContext, null); } finally { @@ -1272,12 +1280,13 @@ public ValueTask SendAsync(HttpRequestMessage request, bool } } - private async ValueTask ConnectToTcpHostAsync(string host, int port, HttpRequestMessage initialRequest, bool async, CancellationToken cancellationToken) + private async ValueTask<(Socket?, Stream)> ConnectToTcpHostAsync(string host, int port, HttpRequestMessage initialRequest, bool async, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); var endPoint = new DnsEndPoint(host, port); Socket? socket = null; + Stream? stream = null; try { // If a ConnectCallback was supplied, use that to establish the connection. @@ -1294,7 +1303,7 @@ private async ValueTask ConnectToTcpHostAsync(string host, int port, Htt Trace($"{nameof(SocketsHttpHandler.ConnectCallback)} completing asynchronously for a synchronous request."); } - return await streamTask.ConfigureAwait(false) ?? throw new HttpRequestException(SR.net_http_null_from_connect_callback); + stream = await streamTask.ConfigureAwait(false) ?? throw new HttpRequestException(SR.net_http_null_from_connect_callback); } else { @@ -1313,8 +1322,10 @@ private async ValueTask ConnectToTcpHostAsync(string host, int port, Htt } } - return new NetworkStream(socket, ownsSocket: true); + stream = new NetworkStream(socket, ownsSocket: true); } + + return (socket, stream); } catch (Exception ex) { @@ -1327,7 +1338,7 @@ private async ValueTask ConnectToTcpHostAsync(string host, int port, Htt internal async ValueTask<(HttpConnection?, HttpResponseMessage?)> CreateHttp11ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) { - (Stream? stream, TransportContext? transportContext, HttpResponseMessage? failureResponse) = + (Socket? socket, Stream? stream, TransportContext? transportContext, HttpResponseMessage? failureResponse) = await ConnectAsync(request, async, cancellationToken).ConfigureAwait(false); if (failureResponse != null) @@ -1335,7 +1346,7 @@ private async ValueTask ConnectToTcpHostAsync(string host, int port, Htt return (null, failureResponse); } - return (await ConstructHttp11ConnectionAsync(async, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), null); + return (await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), null); } private SslClientAuthenticationOptions GetSslOptionsForRequest(HttpRequestMessage request) @@ -1393,10 +1404,15 @@ private async ValueTask ApplyPlaintextFilterAsync(bool async, Stream str return newStream; } - private async ValueTask ConstructHttp11ConnectionAsync(bool async, Stream stream, TransportContext? transportContext, HttpRequestMessage request, CancellationToken cancellationToken) + private async ValueTask ConstructHttp11ConnectionAsync(bool async, Socket? socket, Stream stream, TransportContext? transportContext, HttpRequestMessage request, CancellationToken cancellationToken) { - stream = await ApplyPlaintextFilterAsync(async, stream, HttpVersion.Version11, request, cancellationToken).ConfigureAwait(false); - return new HttpConnection(this, stream, transportContext); + Stream newStream = await ApplyPlaintextFilterAsync(async, stream, HttpVersion.Version11, request, cancellationToken).ConfigureAwait(false); + if (newStream != stream) + { + // If a plaintext filter created a new stream, we can't trust that the socket is still applicable, so rely on it. + socket = null; + } + return new HttpConnection(this, socket, newStream, transportContext); } private async ValueTask ConstructHttp2ConnectionAsync(Stream stream, HttpRequestMessage request, CancellationToken cancellationToken) From dfc1588442b66705d0e7a1bfbde94f0e3c40861d Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 11 Mar 2021 17:39:34 -0500 Subject: [PATCH 2/2] Update src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Co-authored-by: Cory Nelson --- .../System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6184dd0327383..4b644f6784eed 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 @@ -1409,7 +1409,7 @@ private async ValueTask ConstructHttp11ConnectionAsync(bool asyn Stream newStream = await ApplyPlaintextFilterAsync(async, stream, HttpVersion.Version11, request, cancellationToken).ConfigureAwait(false); if (newStream != stream) { - // If a plaintext filter created a new stream, we can't trust that the socket is still applicable, so rely on it. + // If a plaintext filter created a new stream, we can't trust that the socket is still applicable. socket = null; } return new HttpConnection(this, socket, newStream, transportContext);