diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
index 6f2b2112b9e1af..3b800986d77157 100644
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
@@ -1587,12 +1587,19 @@ public bool IsExpired(long nowTicks,
(_httpStreams.Count == 0) &&
((nowTicks - _idleSinceTickCount) > connectionIdleTimeout.TotalMilliseconds))
{
- if (NetEventSource.Log.IsEnabled()) Trace($"Connection no longer usable. Idle {TimeSpan.FromMilliseconds(nowTicks - _idleSinceTickCount)} > {connectionIdleTimeout}.");
+ if (NetEventSource.Log.IsEnabled()) Trace($"HTTP/2 connection no longer usable. Idle {TimeSpan.FromMilliseconds(nowTicks - _idleSinceTickCount)} > {connectionIdleTimeout}.");
return true;
}
- return LifetimeExpired(nowTicks, connectionLifetime);
+ if (LifetimeExpired(nowTicks, connectionLifetime))
+ {
+ if (NetEventSource.Log.IsEnabled()) Trace($"HTTP/2 connection no longer usable. Lifetime {TimeSpan.FromMilliseconds(nowTicks - CreationTickCount)} > {connectionLifetime}.");
+
+ return true;
+ }
+
+ return false;
}
private void AbortStreams(Exception abortException)
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 9d5aa5dac2d977..fd28f25e89a03d 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
@@ -135,54 +135,84 @@ protected void Dispose(bool disposing)
}
}
- /// Do a non-blocking poll to see whether the connection has data available or has been closed.
- /// If we don't have direct access to the underlying socket, we instead use a read-ahead task.
- public bool PollRead()
+ /// Prepare an idle connection to be used for a new request.
+ /// Indicates whether the coming request will be sync or async.
+ /// True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.
+ public bool PrepareForReuse(bool async)
{
- if (_socket != null) // may be null if we don't have direct access to the socket
+ // We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
+ // If the read-ahead task is completed, then we've received either EOF or erroneous data the connection, so it's not usable.
+ if (_readAheadTask is not null)
{
+ return !_readAheadTask.Value.IsCompleted;
+ }
+
+ // 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 (!async && _socket is not null)
+ {
+ // Directly poll the socket rather than doing an async read, so that we can
+ // issue an appropriate sync read when we actually need it.
try
{
- return _socket.Poll(0, SelectMode.SelectRead);
+ return !_socket.Poll(0, SelectMode.SelectRead);
}
catch (Exception e) when (e is SocketException || e is ObjectDisposedException)
{
// Poll can throw when used on a closed socket.
- return true;
+ return false;
}
}
else
{
- return EnsureReadAheadAndPollRead();
+ // 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.
+ try
+ {
+#pragma warning disable CA2012 // we're very careful to ensure the ValueTask is only consumed once, even though it's stored into a field
+ _readAheadTask = _stream.ReadAsync(new Memory(_readBuffer));
+#pragma warning restore CA2012
+ return !_readAheadTask.Value.IsCompleted;
+ }
+ catch (Exception error)
+ {
+ // If reading throws, eat the error and don't reuse the connection.
+ if (NetEventSource.Log.IsEnabled()) Trace($"Error performing read ahead: {error}");
+ return false;
+ }
}
}
- ///
- /// Issues a read-ahead on the connection, which will serve both as the first read on the
- /// response as well as a polling indication of whether the connection is usable.
- ///
- /// true if there's data available on the connection or it's been closed; otherwise, false.
- public bool EnsureReadAheadAndPollRead()
+ /// Check whether a currently idle connection is still usable, or should be scavenged.
+ /// True if connection can be used, false if it is invalid due to receiving EOF or unexpected data.
+ public bool CheckUsabilityOnScavenge()
{
- try
+ // We may already have a read-ahead task if we did a previous scavenge and haven't used the connection since.
+ if (_readAheadTask is null)
{
- Debug.Assert(_readAheadTask == null || _socket == null, "Should only already have a read-ahead task if we don't have a socket to poll");
- if (_readAheadTask == null)
- {
#pragma warning disable CA2012 // we're very careful to ensure the ValueTask is only consumed once, even though it's stored into a field
- _readAheadTask = _stream.ReadAsync(new Memory(_readBuffer));
+ _readAheadTask = ReadAheadWithZeroByteReadAsync();
#pragma warning restore CA2012
- }
}
- catch (Exception error)
+
+ // If the read-ahead task is completed, then we've received either EOF or erroneous data the connection, so it's not usable.
+ return !_readAheadTask.Value.IsCompleted;
+
+ async ValueTask ReadAheadWithZeroByteReadAsync()
{
- // If reading throws, eat the error and don't pool the connection.
- if (NetEventSource.Log.IsEnabled()) Trace($"Error performing read ahead: {error}");
- Dispose();
- _readAheadTask = new ValueTask(0);
- }
+ Debug.Assert(_readAheadTask is null);
+ Debug.Assert(RemainingBuffer.Length == 0);
+
+ // Issue a zero-byte read.
+ // If the underlying stream supports it, this will not complete until the stream has data available,
+ // which will avoid pinning the connection's read buffer (and possibly allow us to release it to the buffer pool in the future, if desired).
+ // If not, it will complete immediately.
+ await _stream.ReadAsync(Memory.Empty).ConfigureAwait(false);
- return _readAheadTask.Value.IsCompleted; // equivalent to polling
+ // We don't know for sure that the stream actually has data available, so we need to issue a real read now.
+ return await _stream.ReadAsync(new Memory(_readBuffer)).ConfigureAwait(false);
+ }
}
private ValueTask? ConsumeReadAheadTask()
diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs
index 36a57147822a8c..d0e82bc6d46b68 100644
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs
@@ -60,17 +60,13 @@ protected void TraceConnection(Stream stream)
}
}
- private long CreationTickCount { get; } = Environment.TickCount64;
+ public long CreationTickCount { get; } = Environment.TickCount64;
// Check if lifetime expired on connection.
public bool LifetimeExpired(long nowTicks, TimeSpan lifetime)
{
- bool expired =
- lifetime != Timeout.InfiniteTimeSpan &&
+ return lifetime != Timeout.InfiniteTimeSpan &&
(lifetime == TimeSpan.Zero || (nowTicks - CreationTickCount) > lifetime.TotalMilliseconds);
-
- if (expired && NetEventSource.Log.IsEnabled()) Trace($"Connection no longer usable. Alive {TimeSpan.FromMilliseconds((nowTicks - CreationTickCount))} > {lifetime}.");
- return expired;
}
internal static bool IsDigit(byte c) => (uint)(c - '0') <= '9' - '0';
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 4b644f6784eed5..159dff77692f09 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
@@ -396,27 +396,6 @@ 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)
@@ -474,16 +453,22 @@ private static bool IsUsableHttp11Connection(HttpConnection connection, long now
}
}
- if (IsUsableHttp11Connection(connection, nowTicks, _poolManager.Settings._pooledConnectionLifetime, async))
+ if (connection.LifetimeExpired(nowTicks, _poolManager.Settings._pooledConnectionLifetime))
{
- if (NetEventSource.Log.IsEnabled()) connection.Trace("Found usable connection in pool.");
- return new ValueTask(connection);
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Found expired connection in pool.");
+ connection.Dispose();
+ continue;
}
- else
+
+ if (!connection.PrepareForReuse(async))
{
if (NetEventSource.Log.IsEnabled()) connection.Trace("Found invalid connection in pool.");
connection.Dispose();
+ continue;
}
+
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Found usable connection in pool.");
+ return new ValueTask(connection);
}
// We are at the connection limit. Wait for an available connection or connection count.
@@ -716,6 +701,7 @@ private static bool IsUsableHttp11Connection(HttpConnection connection, long now
if (http2Connection.LifetimeExpired(Environment.TickCount64, pooledConnectionLifetime))
{
// Connection expired.
+ if (NetEventSource.Log.IsEnabled()) http2Connection.Trace("Found expired HTTP2 connection.");
http2Connection.Dispose();
InvalidateHttp2Connection(http2Connection);
}
@@ -760,6 +746,7 @@ private void AddHttp2Connection(Http2Connection newConnection)
if (http3Connection.LifetimeExpired(Environment.TickCount64, pooledConnectionLifetime) || http3Connection.Authority != authority)
{
// Connection expired.
+ if (NetEventSource.Log.IsEnabled()) http3Connection.Trace("Found expired HTTP3 connection.");
http3Connection.Dispose();
InvalidateHttp3Connection(http3Connection);
}
@@ -1470,24 +1457,6 @@ private async ValueTask ConstructHttp2ConnectionAsync(Stream st
return waiter;
}
- private bool HasWaiter()
- {
- Debug.Assert(Monitor.IsEntered(SyncObj));
-
- return (_waiters != null && _waiters.Count > 0);
- }
-
- /// Dequeues a waiter from the waiters list. The list must not be empty.
- /// The dequeued waiter.
- private TaskCompletionSourceWithCancellation DequeueWaiter()
- {
- Debug.Assert(Monitor.IsEntered(SyncObj));
- Debug.Assert(Settings._maxConnectionsPerServer != int.MaxValue);
- Debug.Assert(_idleConnections.Count == 0, $"With {_idleConnections.Count} idle connections, we shouldn't have a waiter.");
-
- return _waiters!.Dequeue();
- }
-
private void IncrementConnectionCountNoLock()
{
Debug.Assert(Monitor.IsEntered(SyncObj), $"Expected to be holding {nameof(SyncObj)}");
@@ -1513,9 +1482,16 @@ private bool TransferConnection(HttpConnection? connection)
{
Debug.Assert(Monitor.IsEntered(SyncObj));
- while (HasWaiter())
+ if (_waiters == null)
+ {
+ return false;
+ }
+
+ Debug.Assert(_maxConnections != int.MaxValue, "_waiters queue is allocated but no connection limit is set??");
+
+ while (_waiters.TryDequeue(out TaskCompletionSourceWithCancellation? waiter))
{
- TaskCompletionSource waiter = DequeueWaiter();
+ Debug.Assert(_idleConnections.Count == 0, $"With {_idleConnections.Count} idle connections, we shouldn't have a waiter.");
// Try to complete the task. If it's been cancelled already, this will fail.
if (waiter.TrySetResult(connection))
@@ -1562,61 +1538,51 @@ public void DecrementConnectionCount()
/// The connection to return.
public void ReturnConnection(HttpConnection connection)
{
- bool lifetimeExpired = connection.LifetimeExpired(Environment.TickCount64, _poolManager.Settings._pooledConnectionLifetime);
+ if (connection.LifetimeExpired(Environment.TickCount64, _poolManager.Settings._pooledConnectionLifetime))
+ {
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection return to pool. Connection lifetime expired.");
+ connection.Dispose();
+ return;
+ }
- if (!lifetimeExpired)
+ List list = _idleConnections;
+ lock (SyncObj)
{
- List list = _idleConnections;
- lock (SyncObj)
- {
- Debug.Assert(list.Count <= _maxConnections, $"Expected {list.Count} <= {_maxConnections}");
+ Debug.Assert(list.Count <= _maxConnections, $"Expected {list.Count} <= {_maxConnections}");
- // Mark the pool as still being active.
- _usedSinceLastCleanup = true;
+ // Mark the pool as still being active.
+ _usedSinceLastCleanup = true;
- // If there's someone waiting for a connection and this one's still valid, simply transfer this one to them rather than pooling it.
- // Note that while we checked connection lifetime above, we don't check idle timeout, as even if idle timeout
- // is zero, we consider a connection that's just handed from one use to another to never actually be idle.
- bool receivedUnexpectedData = false;
- if (HasWaiter())
- {
- receivedUnexpectedData = connection.EnsureReadAheadAndPollRead();
- if (!receivedUnexpectedData && TransferConnection(connection))
- {
- if (NetEventSource.Log.IsEnabled()) connection.Trace("Transferred connection to waiter.");
- return;
- }
- }
+ // If there's someone waiting for a connection and this one's still valid, simply transfer this one to them rather than pooling it.
+ // Note that while we checked connection lifetime above, we don't check idle timeout, as even if idle timeout
+ // is zero, we consider a connection that's just handed from one use to another to never actually be idle.
+ if (TransferConnection(connection))
+ {
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Transferred connection to waiter.");
+ return;
+ }
- // If the connection is still valid, add it to the list.
+ if (_poolManager.Settings._pooledConnectionIdleTimeout == TimeSpan.Zero)
+ {
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection returned to pool. Zero idle timeout.");
+ }
+ else if (_disposed)
+ {
// If the pool has been disposed of, dispose the connection being returned,
// as the pool is being deactivated. We do this after the above in order to
// use pooled connections to satisfy any requests that pended before the
- // the pool was disposed of. We also dispose of connections if connection
- // timeouts are such that the connection would immediately expire, anyway, as
- // well as for connections that have unexpectedly received extraneous data / EOF.
- if (!receivedUnexpectedData &&
- !_disposed &&
- _poolManager.Settings._pooledConnectionIdleTimeout != TimeSpan.Zero)
- {
- // Pool the connection by adding it to the list.
- list.Add(new CachedConnection(connection));
- if (NetEventSource.Log.IsEnabled()) connection.Trace("Stored connection in pool.");
- return;
- }
+ // the pool was disposed of.
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection returned to pool. Pool was disposed.");
+ }
+ else
+ {
+ // Pool the connection by adding it to the list.
+ list.Add(new CachedConnection(connection));
+ if (NetEventSource.Log.IsEnabled()) connection.Trace("Stored connection in pool.");
+ return;
}
}
- // The connection could be not be reused. Dispose of it.
- // Disposing it will alert any waiters that a connection slot has become available.
- if (NetEventSource.Log.IsEnabled())
- {
- connection.Trace(
- lifetimeExpired ? "Disposing connection return to pool. Connection lifetime expired." :
- _poolManager.Settings._pooledConnectionIdleTimeout == TimeSpan.Zero ? "Disposing connection returned to pool. Zero idle timeout." :
- _disposed ? "Disposing connection returned to pool. Pool was disposed." :
- "Disposing connection returned to pool. Read-ahead unexpectedly completed.");
- }
connection.Dispose();
}
@@ -1941,11 +1907,24 @@ public bool IsUsable(
if ((pooledConnectionIdleTimeout != Timeout.InfiniteTimeSpan) &&
((nowTicks - _returnedTickCount) > pooledConnectionIdleTimeout.TotalMilliseconds))
{
- if (NetEventSource.Log.IsEnabled()) _connection.Trace($"Connection no longer usable. Idle {TimeSpan.FromMilliseconds((nowTicks - _returnedTickCount))} > {pooledConnectionIdleTimeout}.");
+ if (NetEventSource.Log.IsEnabled()) _connection.Trace($"Scavenging connection. Idle {TimeSpan.FromMilliseconds((nowTicks - _returnedTickCount))} > {pooledConnectionIdleTimeout}.");
+ return false;
+ }
+
+ // Validate that the connection lifetime has not been exceeded.
+ if (_connection.LifetimeExpired(nowTicks, pooledConnectionLifetime))
+ {
+ if (NetEventSource.Log.IsEnabled()) _connection.Trace($"Scavenging connection. Lifetime {TimeSpan.FromMilliseconds((nowTicks - _connection.CreationTickCount))} > {pooledConnectionLifetime}.");
+ return false;
+ }
+
+ if (!_connection.CheckUsabilityOnScavenge())
+ {
+ if (NetEventSource.Log.IsEnabled()) _connection.Trace($"Scavenging connection. Unexpected data or EOF received.");
return false;
}
- return IsUsableHttp11Connection(_connection, nowTicks, pooledConnectionLifetime, false);
+ return true;
}
public bool Equals(CachedConnection other) => ReferenceEquals(other._connection, _connection);