From e7fb4e57ae5628285b75a82b54df6b6bf9e15b5e Mon Sep 17 00:00:00 2001
From: Jan Tattermusch <jtattermusch@google.com>
Date: Thu, 23 Aug 2018 15:43:15 +0200
Subject: [PATCH] avoid deadlock while cancelling a call

---
 src/csharp/Grpc.Core/Internal/AsyncCall.cs    | 27 ++++++++++++++--
 .../Grpc.Core/Internal/AsyncCallBase.cs       | 32 ++++++++++++++++---
 .../Grpc.Core/Internal/AsyncCallServer.cs     | 10 ++++--
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/src/csharp/Grpc.Core/Internal/AsyncCall.cs b/src/csharp/Grpc.Core/Internal/AsyncCall.cs
index 3c9e090ba44..66902f3caae 100644
--- a/src/csharp/Grpc.Core/Internal/AsyncCall.cs
+++ b/src/csharp/Grpc.Core/Internal/AsyncCall.cs
@@ -325,9 +325,18 @@ namespace Grpc.Core.Internal
             }
         }
 
-        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 @@ namespace Grpc.Core.Internal
             TResponse msg = default(TResponse);
             var deserializeException = TryDeserialize(receivedMessage, out msg);
 
+            bool releasedResources;
             lock (myLock)
             {
                 finished = true;
@@ -464,7 +474,12 @@ namespace Grpc.Core.Internal
                     streamingWriteTcs = null;
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             responseHeadersTcs.SetResult(responseHeaders);
@@ -494,6 +509,7 @@ namespace Grpc.Core.Internal
 
             TaskCompletionSource<object> delayedStreamingWriteTcs = null;
 
+            bool releasedResources;
             lock (myLock)
             {
                 finished = true;
@@ -504,7 +520,12 @@ namespace Grpc.Core.Internal
                     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
index 3273c26b88f..5a53049e4bd 100644
--- a/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs
+++ b/src/csharp/Grpc.Core/Internal/AsyncCallBase.cs
@@ -196,10 +196,14 @@ namespace Grpc.Core.Internal
                 call.Dispose();
             }
             disposed = true;
-            OnAfterReleaseResources();
+            OnAfterReleaseResourcesLocked();
         }
 
-        protected virtual void OnAfterReleaseResources()
+        protected virtual void OnAfterReleaseResourcesLocked()
+        {
+        }
+
+        protected virtual void OnAfterReleaseResourcesUnlocked()
         {
         }
 
@@ -235,6 +239,7 @@ namespace Grpc.Core.Internal
         {
             bool delayCompletion = false;
             TaskCompletionSource<object> origTcs = null;
+            bool releasedResources;
             lock (myLock)
             {
                 if (!success && !finished && IsClient) {
@@ -252,7 +257,12 @@ namespace Grpc.Core.Internal
                     streamingWriteTcs = null;    
                 }
 
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (!success)
@@ -282,9 +292,15 @@ namespace Grpc.Core.Internal
         /// </summary>
         protected void HandleSendStatusFromServerFinished(bool success)
         {
+            bool releasedResources;
             lock (myLock)
             {
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (!success)
@@ -310,6 +326,7 @@ namespace Grpc.Core.Internal
             var deserializeException = (success && receivedMessage != null) ? TryDeserialize(receivedMessage, out msg) : null;
 
             TaskCompletionSource<TRead> origTcs = null;
+            bool releasedResources;
             lock (myLock)
             {
                 origTcs = streamingReadTcs;
@@ -332,7 +349,12 @@ namespace Grpc.Core.Internal
                     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
index 11acb275334..0ceca4abb8f 100644
--- a/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs
+++ b/src/csharp/Grpc.Core/Internal/AsyncCallServer.cs
@@ -184,7 +184,7 @@ namespace Grpc.Core.Internal
             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 @@ namespace Grpc.Core.Internal
         {
             // 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 @@ namespace Grpc.Core.Internal
                     streamingReadTcs = new TaskCompletionSource<TRequest>();
                     streamingReadTcs.SetResult(default(TRequest));
                 }
-                ReleaseResourcesIfPossible();
+                releasedResources = ReleaseResourcesIfPossible();
+            }
+
+            if (releasedResources)
+            {
+                OnAfterReleaseResourcesUnlocked();
             }
 
             if (cancelled)