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 af64584f02c68..67e6bb5305910 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 @@ -257,6 +257,7 @@ private void Shutdown() if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_shutdown)}={_shutdown}, {nameof(_abortException)}={_abortException}"); Debug.Assert(Monitor.IsEntered(SyncObject)); + Debug.Assert(!_pool.HasSyncObjLock); if (!_shutdown) { @@ -276,6 +277,8 @@ private void Shutdown() public bool TryReserveStream() { + Debug.Assert(!_pool.HasSyncObjLock); + lock (SyncObject) { if (_shutdown) @@ -302,6 +305,8 @@ public bool TryReserveStream() // Otherwise, will be called when the request is complete and stream is closed. public void ReleaseStream() { + Debug.Assert(!_pool.HasSyncObjLock); + lock (SyncObject) { if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_streamsInUse)}={_streamsInUse}"); @@ -333,6 +338,8 @@ public void ReleaseStream() // Returns false to indicate that the connection is shutting down and cannot be used anymore public Task WaitForAvailableStreamsAsync() { + Debug.Assert(!_pool.HasSyncObjLock); + lock (SyncObject) { Debug.Assert(_availableStreamsWaiter is null, "As used currently, shouldn't already have a waiter"); @@ -730,6 +737,7 @@ private void ProcessAltSvcFrame(FrameHeader frameHeader) { if (NetEventSource.Log.IsEnabled()) Trace($"{frameHeader}"); Debug.Assert(frameHeader.Type == FrameType.AltSvc); + Debug.Assert(!Monitor.IsEntered(SyncObject)); ReadOnlySpan span = _incomingBuffer.ActiveSpan.Slice(0, frameHeader.PayloadLength); @@ -1304,6 +1312,8 @@ private Task SendRstStreamAsync(int streamId, Http2ProtocolErrorCode errorCode) internal void HeartBeat() { + Debug.Assert(!_pool.HasSyncObjLock); + if (_shutdown) return; @@ -1810,10 +1820,14 @@ private bool ForceSendConnectionWindowUpdate() public override long GetIdleTicks(long nowTicks) { - lock (SyncObject) - { - return _streamsInUse == 0 ? base.GetIdleTicks(nowTicks) : 0; - } + // The pool is holding the lock as part of its scavenging logic. + // We must not lock on Http2Connection.SyncObj here as that could lead to lock ordering problems. + Debug.Assert(_pool.HasSyncObjLock); + + // There is a race condition here where the connection pool may see this connection as idle right before + // we start processing a new request and start its disposal. This is okay as we will either + // return false from TryReserveStream, or process pending requests before tearing down the transport. + return _streamsInUse == 0 ? base.GetIdleTicks(nowTicks) : 0; } /// Abort all streams and cause further processing to fail. @@ -2002,6 +2016,7 @@ private static TaskCompletionSourceWithCancellation CreateSuccessfullyComp public async Task SendAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) { Debug.Assert(async); + Debug.Assert(!_pool.HasSyncObjLock); if (NetEventSource.Log.IsEnabled()) Trace($"Sending request: {request}"); try 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 c9a6deab2e77f..11e220b9a9e8b 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 @@ -406,7 +406,7 @@ private object SyncObj } } - private bool HasSyncObjLock => Monitor.IsEntered(_availableHttp11Connections); + public bool HasSyncObjLock => Monitor.IsEntered(_availableHttp11Connections); // Overview of connection management (mostly HTTP version independent): // @@ -459,6 +459,8 @@ private static void ThrowGetVersionException(HttpRequestMessage request, int des private bool CheckExpirationOnGet(HttpConnectionBase connection) { + Debug.Assert(!HasSyncObjLock); + TimeSpan pooledConnectionLifetime = _poolManager.Settings._pooledConnectionLifetime; if (pooledConnectionLifetime != Timeout.InfiniteTimeSpan) { @@ -1999,6 +2001,7 @@ private void ReturnHttp2Connection(Http2Connection connection, bool isNewConnect { if (NetEventSource.Log.IsEnabled()) connection.Trace($"{nameof(isNewConnection)}={isNewConnection}"); + Debug.Assert(!HasSyncObjLock); Debug.Assert(isNewConnection || initialRequestWaiter is null, "Shouldn't have a request unless the connection is new"); if (!isNewConnection && CheckExpirationOnReturn(connection)) @@ -2402,6 +2405,7 @@ internal void HeartBeat() localHttp2Connections = _availableHttp2Connections?.ToArray(); } + // Avoid calling HeartBeat under the lock, as it may call back into HttpConnectionPool.InvalidateHttp2Connection. if (localHttp2Connections is not null) { foreach (Http2Connection http2Connection in localHttp2Connections)