Merge pull request grpc#16440 from jtattermusch/csharp_cancellation_d…

…eadlock C#: Avoid deadlock while cancelling a call
thinkerou · Aug 27, 2018 · ca12a87 · ca12a87
2 parents a46b41b + e7fb4e5
commit ca12a87
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 10 deletions.
diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs
@@ -325,9 +325,18 @@ public CallInvocationDetails<TRequest, TResponse> Details
             }
         }
 
-        protected override void OnAfterReleaseResources()
+        protected override void OnAfterReleaseResourcesLocked()
         {
             details.Channel.RemoveCallReference(this);
+        }
+
+        protected override void OnAfterReleaseResourcesUnlocked()
+        {
+            // If cancellation callback is in progress, this can block
+            // so we need to do this outside of call's lock to prevent
+            // deadlock.
+            // See https://github.com/grpc/grpc/issues/14777
+            // See https://github.com/dotnet/corefx/issues/14903
             cancellationTokenRegistration?.Dispose();
         }
 
@@ -448,6 +457,7 @@ private void HandleUnaryResponse(bool success, ClientSideStatus receivedStatus,
             TResponse msg = default(TResponse);
             var deserializeException = TryDeserialize(receivedMessage, out msg);
 
+            bool releasedResources;
             lock (myLock)
             {
                 finished = true;
@@ -464,7 +474,12 @@ private void HandleUnaryResponse(bool success, ClientSideStatus receivedStatus,
                     streamingWriteTcs = null;
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             responseHeadersTcs.SetResult(responseHeaders);
@@ -494,6 +509,7 @@ private void HandleFinished(bool success, ClientSideStatus receivedStatus)
 
             TaskCompletionSource<object> delayedStreamingWriteTcs = null;
 
+            bool releasedResources;
             lock (myLock)
             {
                 finished = true;
@@ -504,7 +520,12 @@ private void HandleFinished(bool success, ClientSideStatus receivedStatus)
                     streamingWriteTcs = null;
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (delayedStreamingWriteTcs != null)

diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs
@@ -196,10 +196,14 @@ private void ReleaseResources()
                 call.Dispose();
             }
             disposed = true;
-            OnAfterReleaseResources();
+            OnAfterReleaseResourcesLocked();
         }
 
-        protected virtual void OnAfterReleaseResources()
+        protected virtual void OnAfterReleaseResourcesLocked()
+        {
+        }
+
+        protected virtual void OnAfterReleaseResourcesUnlocked()
         {
         }
 
@@ -235,6 +239,7 @@ protected void HandleSendFinished(bool success)
         {
             bool delayCompletion = false;
             TaskCompletionSource<object> origTcs = null;
+            bool releasedResources;
             lock (myLock)
             {
                 if (!success && !finished && IsClient) {
@@ -252,7 +257,12 @@ protected void HandleSendFinished(bool success)
                     streamingWriteTcs = null;    
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (!success)
@@ -282,9 +292,15 @@ protected void HandleSendFinished(bool success)
         /// </summary>
         protected void HandleSendStatusFromServerFinished(bool success)
         {
+            bool releasedResources;
             lock (myLock)
             {
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (!success)
@@ -310,6 +326,7 @@ protected void HandleReadFinished(bool success, byte[] receivedMessage)
             var deserializeException = (success && receivedMessage != null) ? TryDeserialize(receivedMessage, out msg) : null;
 
             TaskCompletionSource<TRead> origTcs = null;
+            bool releasedResources;
             lock (myLock)
             {
                 origTcs = streamingReadTcs;
@@ -332,7 +349,12 @@ protected void HandleReadFinished(bool success, byte[] receivedMessage)
                     streamingReadTcs = null;
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (deserializeException != null && !IsClient)

diff --git a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs
@@ -184,7 +184,7 @@ protected override Exception GetRpcExceptionClientOnly()
             throw new InvalidOperationException("Call be only called for client calls");
         }
 
-        protected override void OnAfterReleaseResources()
+        protected override void OnAfterReleaseResourcesLocked()
         {
             server.RemoveCallReference(this);
         }
@@ -206,6 +206,7 @@ private void HandleFinishedServerside(bool success, bool cancelled)
         {
             // NOTE: because this event is a result of batch containing GRPC_OP_RECV_CLOSE_ON_SERVER,
             // success will be always set to true.
+            bool releasedResources;
             lock (myLock)
             {
                 finished = true;
@@ -217,7 +218,12 @@ private void HandleFinishedServerside(bool success, bool cancelled)
                     streamingReadTcs = new TaskCompletionSource<TRequest>();
                     streamingReadTcs.SetResult(default(TRequest));
                 }
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (cancelled)