search

cyanfish / grpc-dotnet-namedpipes

Named pipe transport for gRPC in C#/.NET
Apache License 2.0
190 stars 48 forks source link

Race condition in PayloadQueue.MoveNext #20

Closed ccruden-shookiot closed 3 years ago

ccruden-shookiot commented 3 years ago

Under certain circumstances, the MoveNext operation can attempt to cancel an operation that it's already completed.

15:09:12 Application: TagsService.exe CoreCLR Version: 5.0.321.7212 .NET Version: 5.0.3 Description: The process was terminated due to an unhandled exception. Exception Info: System.AggregateException: One or more errors occurred. (One or more errors occurred. (One or more errors occurred. (An attempt was made to transition a task to a final state when it had already completed.))) ---> System.AggregateException: One or more errors occurred. (One or more errors occurred. (An attempt was made to transition a task to a final state when it had already completed.)) ---> System.AggregateException: One or more errors occurred. (An attempt was made to transition a task to a final state when it had already completed.) ---> System.InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed. at System.Threading.Tasks.TaskCompletionSource1.SetCanceled(CancellationToken cancellationToken) at System.Threading.Tasks.TaskCompletionSource1.SetCanceled() at GrpcDotNetNamedPipes.Internal.PayloadQueue.b__9_0() at System.Threading.CancellationToken.<>c.<.cctor>b__26_0(Object obj) at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) --- End of stack trace from previous location --- at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) --- End of inner exception stack trace --- at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) --- End of inner exception stack trace --- at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) --- End of inner exception stack trace --- at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException) at System.Threading.TimerQueueTimer.CallCallback(Boolean isThreadPool) at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool) at System.Threading.TimerQueue.FireNextTimers()

The cancellation token passed in to MoveNext was on a timer (CancellationTokenSource.CancelAfter). I think in this case what happened was _tcs.SetResult() got called and, before ResetTcs() could get called, context got switched to the timer thread, which fired the cancellation token and tried to call SetCanceled on the already set completion source.

At a guess, all you'd need to do to prevent this particular problem is wrap _tcs.SetCanceled() in a try / catch (InvalidOperationException) or use TrySetCanceled() instead. May want to replace other calls to SetResult with TrySetResult as well to prevent the same thing happening in reverse.

cyanfish commented 3 years ago

Thanks for catching this! And sorry for the delay in responding. Should be fixed in https://github.com/cyanfish/grpc-dotnet-namedpipes/commit/51126a7a655e459696a67edb15a493601f7856b6 and published as version 1.4.0. Let me know if that works for you.

ccruden-shookiot commented 3 years ago

Awesome! That's great! Thanks very much!