From 8e943c5e1513a816777961a8adf0151c189d181a Mon Sep 17 00:00:00 2001 From: Geoff Kizer Date: Sun, 21 Feb 2021 16:17:48 -0800 Subject: [PATCH] Simplify connection usability logic (#48446) * simplify connection usability logic Co-authored-by: Geoffrey Kizer --- .../SocketsHttpHandler/HttpConnectionPool.cs | 95 ++++++++----------- 1 file changed, 39 insertions(+), 56 deletions(-) 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 fd03e3d5c68..c60e5ca8a35 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 @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -14,7 +13,6 @@ using System.Net.Security; using System.Net.Sockets; using System.Runtime.CompilerServices; -using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Security.Authentication; using System.Text; @@ -398,6 +396,27 @@ public byte[] Http2AltSvcOriginUri return GetHttpConnectionAsync(request, async, cancellationToken); } + private static bool IsUsableHttp11Connection(HttpConnection connection, long nowTicks, TimeSpan lifetime, bool async) + { + if (connection.LifetimeExpired(nowTicks, lifetime)) + { + return false; + } + + // Check to see if we've received anything on the connection; if we have, that's + // either erroneous data (we shouldn't have received anything yet) or the connection + // has been closed; either way, we can't use it. If this is an async request, we + // perform an async read on the stream, since we're going to need to read from it + // anyway, and in doing so we can avoid the extra syscall. For sync requests, we + // try to directly poll the socket rather than doing an async read, so that we can + // issue an appropriate sync read when we actually need it. We don't have the + // underlying socket in all cases, though, so PollRead may fall back to an async + // read in some cases. + return async ? + !connection.EnsureReadAheadAndPollRead() : + !connection.PollRead(); + } + private ValueTask GetOrReserveHttp11ConnectionAsync(bool async, CancellationToken cancellationToken) { if (cancellationToken.IsCancellationRequested) @@ -405,25 +424,24 @@ public byte[] Http2AltSvcOriginUri return ValueTask.FromCanceled(cancellationToken); } - TimeSpan pooledConnectionLifetime = _poolManager.Settings._pooledConnectionLifetime; + List idleConnections = _idleConnections; long nowTicks = Environment.TickCount64; - List list = _idleConnections; - // Try to find a usable cached connection. + // 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) { - CachedConnection cachedConnection; + HttpConnection connection; lock (SyncObj) { - if (list.Count > 0) + if (idleConnections.Count > 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. - cachedConnection = list[list.Count - 1]; - list.RemoveAt(list.Count - 1); + connection = idleConnections[idleConnections.Count - 1]._connection; + idleConnections.RemoveAt(idleConnections.Count - 1); } else { @@ -456,35 +474,16 @@ public byte[] Http2AltSvcOriginUri } } - HttpConnection conn = cachedConnection._connection; - if (!conn.LifetimeExpired(nowTicks, pooledConnectionLifetime)) + if (IsUsableHttp11Connection(connection, nowTicks, _poolManager.Settings._pooledConnectionLifetime, async)) { - // Check to see if we've received anything on the connection; if we have, that's - // either erroneous data (we shouldn't have received anything yet) or the connection - // has been closed; either way, we can't use it. If this is an async request, we - // perform an async read on the stream, since we're going to need to read from it - // anyway, and in doing so we can avoid the extra syscall. For sync requests, we - // try to directly poll the socket rather than doing an async read, so that we can - // issue an appropriate sync read when we actually need it. We don't have the - // underlying socket in all cases, though, so PollRead may fall back to an async - // read in some cases. - bool validConnection = async ? - !conn.EnsureReadAheadAndPollRead() : - !conn.PollRead(); - - if (validConnection) - { - // We found a valid connection. Return it. - if (NetEventSource.Log.IsEnabled()) conn.Trace("Found usable connection in pool."); - return new ValueTask(conn); - } + if (NetEventSource.Log.IsEnabled()) connection.Trace("Found usable connection in pool."); + return new ValueTask(connection); + } + else + { + if (NetEventSource.Log.IsEnabled()) connection.Trace("Found invalid connection in pool."); + connection.Dispose(); } - - // We got a connection, but it was already closed by the server or the - // server sent unexpected data or the connection is too old. In any case, - // we can't use the connection, so get rid of it and loop around to try again. - if (NetEventSource.Log.IsEnabled()) conn.Trace("Found invalid connection in pool."); - conn.Dispose(); } // We are at the connection limit. Wait for an available connection or connection count. @@ -1773,7 +1772,7 @@ public bool CleanCacheAndDisposeIfUnused() // Find the first item which needs to be removed. int freeIndex = 0; - while (freeIndex < list.Count && list[freeIndex].IsUsable(nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout, poll: true)) + while (freeIndex < list.Count && list[freeIndex].IsUsable(nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout)) { freeIndex++; } @@ -1791,7 +1790,7 @@ public bool CleanCacheAndDisposeIfUnused() { // Look for the first item to be kept. Along the way, any // that shouldn't be kept are disposed of. - while (current < list.Count && !list[current].IsUsable(nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout, poll: true)) + while (current < list.Count && !list[current].IsUsable(nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout)) { toDispose.Add(list[current]._connection); current++; @@ -1910,7 +1909,6 @@ public CachedConnection(HttpConnection connection) /// The current tick count. Passed in to amortize the cost of calling Environment.TickCount. /// How long a connection can be open to be considered reusable. /// How long a connection can have been idle in the pool to be considered reusable. - /// true to poll the connection to check if it's usable; otherwise, false. Defaults to false. /// /// true if we believe the connection can be reused; otherwise, false. There is an inherent race condition here, /// in that the server could terminate the connection or otherwise make it unusable immediately after we check it, @@ -1921,8 +1919,7 @@ public CachedConnection(HttpConnection connection) public bool IsUsable( long nowTicks, TimeSpan pooledConnectionLifetime, - TimeSpan pooledConnectionIdleTimeout, - bool poll = false) + TimeSpan pooledConnectionIdleTimeout) { // Validate that the connection hasn't been idle in the pool for longer than is allowed. if ((pooledConnectionIdleTimeout != Timeout.InfiniteTimeSpan) && @@ -1932,21 +1929,7 @@ public CachedConnection(HttpConnection connection) return false; } - // Validate that the connection hasn't been alive for longer than is allowed. - if (_connection.LifetimeExpired(nowTicks, pooledConnectionLifetime)) - { - return false; - } - - // Validate that the connection hasn't received any stray data while in the pool. - if (poll && _connection.PollRead()) - { - if (NetEventSource.Log.IsEnabled()) _connection.Trace($"Connection no longer usable. Unexpected data received."); - return false; - } - - // The connection is usable. - return true; + return IsUsableHttp11Connection(_connection, nowTicks, pooledConnectionLifetime, false); } public bool Equals(CachedConnection other) => ReferenceEquals(other._connection, _connection); -- GitLab