Race condition when disposing of channels while cancelling calls.

[amanda-tarafa]

I think there might be a race condition if a channel is disposed at the same time that one of its active calls is cancelled separately. I think this happens only for server or bidi streaming calls. The original report (googleapis/google-cloud-dotnet#10318) comes from a Google.Cloud.PubSub.V1 user who has provided a repro and logs. A couple of us on the Google side haven't been able to reproduce but I think my description below of the race conditions is accurate, and it's also supported by the user provided logs (ignoring the Google.Cloud.PubSub.V1 part of the logs.)

• Grpc.Net.Client.GrpcChannel.Dispose disposes of active calls within a lock.
  

  grpc-dotnet/src/Grpc.Net.Client/GrpcChannel.cs

  Lines 741 to 754 in 38738af

  	lock (_lock)
  	{
  	if (ActiveCalls.Count > 0)
  	{
  	// Disposing a call will remove it from ActiveCalls. Need to take a copy
  	// to avoid enumeration from being modified
  	var activeCallsCopy = ActiveCalls.ToArray();
  	foreach (var activeCall in activeCallsCopy)
  	{
  	activeCall.Dispose();
  	}
  	}
  	}

• Grpc.Net.Client.GrpcCall.Dispose basically calls GrpcCall.Cleanup with a status of cancel.

• GrpcCall.Cleanup then sets the call TaskCompletionSource to cancelled.
  

  grpc-dotnet/src/Grpc.Net.Client/Internal/GrpcCall.cs

  Line 211 in 38738af

  _callTcs.TrySetResult(status);

• But, if this GrpcCall was already being cancelled elsewhere (i.e. in a different thread) setting the TaskCompletionSource to cancelled blocks until the cancellation callbacks have completed elsewhere. Note that this blocks holding the chanell lock.

In the meantime the thread that first cancelled the GrpcCall has been able to:

• Execute GrpcCall.CancelCallFromCancellationToken which is the callback for the call cancellation token.
• CancelCallFromCancellationToken calls CancelCall which (same as Cleanup) sets the GrpcCall TaskCompletionSource to cancelled. The difference here is that this call doesn't block as it's being executed on the same thread (and as part of) the cancellation callback, so the GrpcCall.Task here is actually transtioned to cancelled.
• But note that a TaskCompletionSource is being used explicitly so that it's continuations are executed inmediately.
  

  grpc-dotnet/src/Grpc.Net.Client/Internal/GrpcCall.cs

  Lines 73 to 74 in 38738af

  	// Run the callTcs continuation immediately to keep the same context. Required for Activity.
  	_callTcs = new TaskCompletionSource<Status>();

• Which means that as soon as the GrpcCall.Task is transitioned to cancelled, and in the same context (thread) an OperationCancelledException is thrown and handled in GrpcCall.RunAsync because we are awaiting for the GrpcCall.Task to complete (in the case of server and bidi streaming).
  

  grpc-dotnet/src/Grpc.Net.Client/Internal/GrpcCall.cs

  Line 621 in 38738af

  status = await CallTask.ConfigureAwait(false);

• Handling of that exception ultimately calls Cleanup
  

  grpc-dotnet/src/Grpc.Net.Client/Internal/GrpcCall.cs

  Line 639 in 38738af

  Cleanup(status.Value);

  
  which in turn calls GrpcChannel.FinishActiveCall
  

  grpc-dotnet/src/Grpc.Net.Client/Internal/GrpcCall.cs

  Line 218 in 38738af

  Channel.FinishActiveCall(this);

  
  which requires the channel lock that is being held by the channel disposing thread, which is waiting for this thread to finish executing the cancellation callback:
  

  grpc-dotnet/src/Grpc.Net.Client/GrpcChannel.cs

  Lines 490 to 501 in 38738af

  	internal void FinishActiveCall(IDisposable grpcCall)
  	{
  	lock (_lock)
  	{
  	ActiveCalls.Remove(grpcCall);
  	}
  	}
  	internal GrpcMethodInfo GetCachedGrpcMethodInfo(IMethod method)
  	{
  	return _methodInfoCache.GetOrAdd(method, _createMethodInfoFunc);
  	}

Possible solution:

In GrpcChannel.Dispose, release the lock inmediately after getting the local copy of the active calls collection. This will avoid the race condition and as far as I can see will introduce no side effect.

• Note that new calls can currently be added to the channel active call collection right after the old calls have been disposed. By releasing the lock earlier we are just allowing that to possibly happen while we are disposing of the old calls. But this wouldn't affect old call disposal as we have a local copy of the active calls collection.
• The call that would otherwise be involved in the race condition will be removed from the channel active call collection twice, but that collection is a Hashset anyway which does nothing if the element being removed is not present.

[amanda-tarafa]

@JamesNK we were about to release a patched beta of Google.Cloud.PubSub.V1 . But if the release of this is more or less inminint, we'd probably want to wait. Do you have a rough idea on when this will be released? Thanks!

[JamesNK]

I'm going to start the 2.54.0 release process, but it will take a couple of weeks to have a preview release and then a final release.