Fix Http2 deadlock (#99889)

dotnet · Mar 21, 2024 · 9b8b01b · 9b8b01b
1 parent 3caa596
commit 9b8b01b
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 5 deletions.
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
@@ -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<bool> 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<byte> 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;
         }
 
         /// <summary>Abort all streams and cause further processing to fail.</summary>
@@ -2002,6 +2016,7 @@ private static TaskCompletionSourceWithCancellation<bool> CreateSuccessfullyComp
         public async Task<HttpResponseMessage> 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
@@ -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)