From 3cff3bedd76e18cad98ac1c7246dd0cc004ca811 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 20 Mar 2020 15:42:43 -0400 Subject: [PATCH] Fix cancellation-related deadlock in BoundedChannel If: - a BoundedChannel is opted-in to synchronous continuations via someone setting AllowSynchronousContinuations on its options when constructed, and - if a read operation is performed when there's no data available and with a cancelable cancellation token, and - if cancellation is requested concurrently with a write being performed that will satisfy that read... then there's the potential for a deadlock. While holding a lock, the writer needs to unregister the cancellation for the reader's operation prior to trying to complete it, and needs to know that all cancellation-related work has quiesced before it proceeds to try to complete that reader. If the cancellation is currently in progress then, it waits for the cancellation callback to complete. However, if a synchronous continuation was used, then it's possible the user's synchronous continuation might call back into the bounded channel and perform an operation that might require taking that same lock. Deadlock. The fix is to simply not try to use synchronous continuations with bounded channel when a cancelable token is employed. --- .../Threading/Channels/BoundedChannel.cs | 22 +++++++++++++++---- .../tests/BoundedChannelTests.cs | 12 +++++----- .../tests/ChannelTestBase.cs | 6 +++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Threading.Channels/src/System/Threading/Channels/BoundedChannel.cs b/src/libraries/System.Threading.Channels/src/System/Threading/Channels/BoundedChannel.cs index 3f7a967a60a47..6e911a797121f 100644 --- a/src/libraries/System.Threading.Channels/src/System/Threading/Channels/BoundedChannel.cs +++ b/src/libraries/System.Threading.Channels/src/System/Threading/Channels/BoundedChannel.cs @@ -146,8 +146,15 @@ public override ValueTask ReadAsync(CancellationToken cancellationToken) } } - // Otherwise, queue the reader. - var reader = new AsyncOperation(parent._runContinuationsAsynchronously, cancellationToken); + // Otherwise, queue a reader. Note that in addition to checking whether synchronous continuations were requested, + // we also check whether the supplied cancellation token can be canceled. The writer calls UnregisterCancellation + // while holding the lock, and if a callback needs to be unregistered and is currently running, it needs to wait + // for that callback to complete so that the subsequent code knows it won't be contending with another thread + // trying to complete the operation. However, if we allowed a synchronous continuation from this operation, that + // cancellation callback could end up running arbitrary code, including code that called back into the reader or + // writer and tried to take the same lock held by the thread running UnregisterCancellation... deadlock. As such, + // we only allow synchronous continuations here if both a) the caller requested it and the token isn't cancelable. + var reader = new AsyncOperation(parent._runContinuationsAsynchronously | cancellationToken.CanBeCanceled, cancellationToken); parent._blockedReaders.EnqueueTail(reader); return reader.ValueTaskOfT; } @@ -193,8 +200,15 @@ public override ValueTask WaitToReadAsync(CancellationToken cancellationTo } } - // Otherwise, queue a reader. - var waiter = new AsyncOperation(parent._runContinuationsAsynchronously, cancellationToken); + // Otherwise, queue a reader. Note that in addition to checking whether synchronous continuations were requested, + // we also check whether the supplied cancellation token can be canceled. The writer calls UnregisterCancellation + // while holding the lock, and if a callback needs to be unregistered and is currently running, it needs to wait + // for that callback to complete so that the subsequent code knows it won't be contending with another thread + // trying to complete the operation. However, if we allowed a synchronous continuation from this operation, that + // cancellation callback could end up running arbitrary code, including code that called back into the reader or + // writer and tried to take the same lock held by the thread running UnregisterCancellation... deadlock. As such, + // we only allow synchronous continuations here if both a) the caller requested it and the token isn't cancelable. + var waiter = new AsyncOperation(parent._runContinuationsAsynchronously | cancellationToken.CanBeCanceled, cancellationToken); ChannelUtilities.QueueWaiter(ref _parent._waitingReadersTail, waiter); return waiter.ValueTaskOfT; } diff --git a/src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs b/src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs index ea27c1482901b..19c28a248fc8e 100644 --- a/src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs +++ b/src/libraries/System.Threading.Channels/tests/BoundedChannelTests.cs @@ -390,16 +390,18 @@ public async Task WaitToWriteAsync_AfterFullThenRead_ReturnsTrue() } [Theory] - [InlineData(false)] - [InlineData(true)] - public void AllowSynchronousContinuations_WaitToReadAsync_ContinuationsInvokedAccordingToSetting(bool allowSynchronousContinuations) + [MemberData(nameof(ThreeBools))] + public void AllowSynchronousContinuations_Reading_ContinuationsInvokedAccordingToSetting(bool allowSynchronousContinuations, bool cancelable, bool waitToReadAsync) { var c = Channel.CreateBounded(new BoundedChannelOptions(1) { AllowSynchronousContinuations = allowSynchronousContinuations }); + CancellationToken ct = cancelable ? new CancellationTokenSource().Token : CancellationToken.None; + int expectedId = Environment.CurrentManagedThreadId; - Task r = c.Reader.WaitToReadAsync().AsTask().ContinueWith(_ => + Task t = waitToReadAsync ? (Task)c.Reader.WaitToReadAsync(ct).AsTask() : c.Reader.ReadAsync(ct).AsTask(); + Task r = t.ContinueWith(_ => { - Assert.Equal(allowSynchronousContinuations, expectedId == Environment.CurrentManagedThreadId); + Assert.Equal(allowSynchronousContinuations && !cancelable, expectedId == Environment.CurrentManagedThreadId); }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); Assert.True(c.Writer.WriteAsync(42).IsCompletedSuccessfully); diff --git a/src/libraries/System.Threading.Channels/tests/ChannelTestBase.cs b/src/libraries/System.Threading.Channels/tests/ChannelTestBase.cs index 42721f12c4bea..ed88b1bed9fcc 100644 --- a/src/libraries/System.Threading.Channels/tests/ChannelTestBase.cs +++ b/src/libraries/System.Threading.Channels/tests/ChannelTestBase.cs @@ -24,6 +24,12 @@ public abstract partial class ChannelTestBase : TestBase protected virtual bool RequiresSingleWriter => false; protected virtual bool BuffersItems => true; + public static IEnumerable ThreeBools => + from b1 in new[] { false, true } + from b2 in new[] { false, true } + from b3 in new[] { false, true } + select new object[] { b1, b2, b3 }; + [Fact] public void ValidateDebuggerAttributes() {