fix deadlock in Quic (#56600)

* fix deadlock in Quic

* feedback from review

diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
index d909514..82638fc 100644 (file)
--- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
+++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
@@ -150,6 +150,7 @@ namespace System.Net.Quic.Implementations.MsQuic
 
             try
             {
+                Debug.Assert(!Monitor.IsEntered(_state));
                 MsQuicApi.Api.SetCallbackHandlerDelegate(
                     _state.Handle,
                     s_connectionDelegate,
@@ -184,6 +185,7 @@ namespace System.Net.Quic.Implementations.MsQuic
             _state.StateGCHandle = GCHandle.Alloc(_state);
             try
             {
+                Debug.Assert(!Monitor.IsEntered(_state));
                 uint status = MsQuicApi.Api.ConnectionOpenDelegate(
                     MsQuicApi.Api.Registration,
                     s_connectionDelegate,
@@ -220,7 +222,7 @@ namespace System.Net.Quic.Implementations.MsQuic
             if (!state.Connected)
             {
                 // Connected will already be true for connections accepted from a listener.
-
+                Debug.Assert(!Monitor.IsEntered(state));
                 SOCKADDR_INET inetAddress = MsQuicParameterHelpers.GetINetParam(MsQuicApi.Api, state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_ADDRESS);
 
                 Debug.Assert(state.Connection != null);
@@ -459,6 +461,10 @@ namespace System.Net.Quic.Implementations.MsQuic
             TaskCompletionSource? tcs = _state.NewUnidirectionalStreamsAvailable;
             if (tcs is null)
             {
+                // We need to avoid calling MsQuic under lock.
+                // This is not atomic but it won't be anyway as counts can change between when task is completed
+                // and before somebody may try to allocate new stream.
+                int count = GetRemoteAvailableUnidirectionalStreamCount();
                 lock (_state)
                 {
                     if (_state.NewUnidirectionalStreamsAvailable is null)
@@ -468,13 +474,14 @@ namespace System.Net.Quic.Implementations.MsQuic
                             throw new QuicOperationAbortedException();
                         }
 
-                        if (GetRemoteAvailableUnidirectionalStreamCount() > 0)
+                        if (count > 0)
                         {
                             return ValueTask.CompletedTask;
                         }
 
                         _state.NewUnidirectionalStreamsAvailable = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
                     }
+
                     tcs = _state.NewUnidirectionalStreamsAvailable;
                 }
             }
@@ -487,6 +494,10 @@ namespace System.Net.Quic.Implementations.MsQuic
             TaskCompletionSource? tcs = _state.NewBidirectionalStreamsAvailable;
             if (tcs is null)
             {
+                // We need to avoid calling MsQuic under lock.
+                // This is not atomic but it won't be anyway as counts can change between when task is completed
+                // and before somebody may try to allocate new stream.
+                int count = GetRemoteAvailableBidirectionalStreamCount();
                 lock (_state)
                 {
                     if (_state.NewBidirectionalStreamsAvailable is null)
@@ -496,7 +507,7 @@ namespace System.Net.Quic.Implementations.MsQuic
                             throw new QuicOperationAbortedException();
                         }
 
-                        if (GetRemoteAvailableBidirectionalStreamCount() > 0)
+                        if (count > 0)
                         {
                             return ValueTask.CompletedTask;
                         }
@@ -526,11 +537,13 @@ namespace System.Net.Quic.Implementations.MsQuic
 
         internal override int GetRemoteAvailableUnidirectionalStreamCount()
         {
+            Debug.Assert(!Monitor.IsEntered(_state));
             return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_UNIDI_STREAM_COUNT);
         }
 
         internal override int GetRemoteAvailableBidirectionalStreamCount()
         {
+            Debug.Assert(!Monitor.IsEntered(_state));
             return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_BIDI_STREAM_COUNT);
         }
 
@@ -563,6 +576,7 @@ namespace System.Net.Quic.Implementations.MsQuic
                 SOCKADDR_INET address = MsQuicAddressHelpers.IPEndPointToINet((IPEndPoint)_remoteEndPoint);
                 unsafe
                 {
+                    Debug.Assert(!Monitor.IsEntered(_state));
                     status = MsQuicApi.Api.SetParamDelegate(_state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.REMOTE_ADDRESS, (uint)sizeof(SOCKADDR_INET), (byte*)&address);
                     QuicExceptionHelpers.ThrowIfFailed(status, "Failed to connect to peer.");
                 }
@@ -587,6 +601,7 @@ namespace System.Net.Quic.Implementations.MsQuic
 
             try
             {
+                Debug.Assert(!Monitor.IsEntered(_state));
                 status = MsQuicApi.Api.ConnectionStartDelegate(
                     _state.Handle,
                     _configuration,
@@ -620,6 +635,7 @@ namespace System.Net.Quic.Implementations.MsQuic
 
             try
             {
+                Debug.Assert(!Monitor.IsEntered(_state));
                 MsQuicApi.Api.ConnectionShutdownDelegate(
                     _state.Handle,
                     Flags,
@@ -747,6 +763,7 @@ namespace System.Net.Quic.Implementations.MsQuic
             if (_state.Handle != null)
             {
                 // Handle can be null if outbound constructor failed and we are called from finalizer.
+                Debug.Assert(!Monitor.IsEntered(_state));
                 MsQuicApi.Api.ConnectionShutdownDelegate(
                     _state.Handle,
                     QUIC_CONNECTION_SHUTDOWN_FLAGS.SILENT,