From 6a94681e3ae32fc665c62c1f77ff1d6ba293ffad Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 25 Jan 2018 17:06:44 -0800 Subject: [PATCH 1/4] Add an unit test to reproduce a dead lock issue. --- .../AsyncReaderWriterLockTests.cs | 74 ++++++++++++++++++- 1 file changed, 72 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 27fd90873..19b822bf2 100644 --- a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2509,9 +2509,79 @@ public async Task CancelNonImpactfulToIssuedLocks() Assert.False(this.asyncLock.IsWriteLockHeld); } -#endregion + [StaFact] + public async Task CancelAfterIsCompletedNoLeak() + { + var lockAwaitFinished = new TaskCompletionSource(); + var testCompleted = new TaskCompletionSource(); + var cts = new CancellationTokenSource(); + + Thread staThread = new Thread((ThreadStart)delegate + { + try + { + var awaitable = this.asyncLock.UpgradeableReadLockAsync(cts.Token); + var awaiter = awaitable.GetAwaiter(); + cts.Cancel(); + + if (awaiter.IsCompleted) + { + try + { + awaiter.GetResult().Dispose(); + Assert.True(false, "The lock should not be issued on an STA thread."); + } + catch (OperationCanceledException) + { + + } + + lockAwaitFinished.SetAsync(); + } + else + { + awaiter.OnCompleted(delegate + { + Assert.Equal(ApartmentState.MTA, Thread.CurrentThread.GetApartmentState()); + try + { + awaiter.GetResult().Dispose(); + } + catch (OperationCanceledException) + { + } + + lockAwaitFinished.SetAsync(); + }); + } + + lockAwaitFinished.Task.Wait(); + + // No lock is leaked + awaitable = this.asyncLock.UpgradeableReadLockAsync(); + awaiter = awaitable.GetAwaiter(); + Assert.False(awaiter.IsCompleted, "The lock should not be issued on an STA thread."); + awaiter.OnCompleted(delegate + { + Assert.Equal(ApartmentState.MTA, Thread.CurrentThread.GetApartmentState()); + awaiter.GetResult().Dispose(); + testCompleted.SetAsync(); + }); + } + catch (Exception ex) + { + testCompleted.TrySetException(ex); + } + }); + + staThread.SetApartmentState(ApartmentState.STA); + staThread.Start(); + await testCompleted.Task; + } + + #endregion -#region Completion tests + #region Completion tests #if DESKTOP || NETCOREAPP2_0 [StaFact] From 5a845ab784b6c592f2af1547a8ec3c3ffe9b5523 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 25 Jan 2018 17:30:26 -0800 Subject: [PATCH 2/4] Apply a fix to address a race condition to cause lock to be leaked This will block new locks to be issued, and easily causes products to run into dead locks. --- .../AsyncReaderWriterLockTests.cs | 3 +-- .../AsyncReaderWriterLock.cs | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 19b822bf2..722e6404a 100644 --- a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2510,7 +2510,7 @@ public async Task CancelNonImpactfulToIssuedLocks() } [StaFact] - public async Task CancelAfterIsCompletedNoLeak() + public async Task CancelJustBeforeIsCompletedNoLeak() { var lockAwaitFinished = new TaskCompletionSource(); var testCompleted = new TaskCompletionSource(); @@ -2533,7 +2533,6 @@ public async Task CancelAfterIsCompletedNoLeak() } catch (OperationCanceledException) { - } lockAwaitFinished.SetAsync(); diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index b11543574..57ae59a73 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -2160,7 +2160,21 @@ internal Awaiter(AsyncReaderWriterLock lck, LockKind kind, LockFlags options, Ca /// public bool IsCompleted { - get { return this.cancellationToken.IsCancellationRequested || this.fault != null || this.LockIssued; } + get + { + if (this.fault != null) + { + return true; + } + + // If lock has already been issued, we have to switch to the right context, and ignore the CancellationToken. + if (this.lck.IsLockActive(this, considerStaActive: true)) + { + return this.lck.IsLockSupportingContext(this); + } + + return this.cancellationToken.IsCancellationRequested; + } } /// From 28d92e79faa3591d0a7adeae06601c017eea4299 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 25 Jan 2018 17:53:07 -0800 Subject: [PATCH 3/4] Add a new unit test to reproduce another CancellationToken related dead lock. --- .../AsyncReaderWriterLockTests.cs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 722e6404a..4cd643daf 100644 --- a/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/src/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2578,6 +2578,45 @@ public async Task CancelJustBeforeIsCompletedNoLeak() await testCompleted.Task; } + [StaFact] + public async Task CancelJustAfterIsCompleted() + { + var lockAwaitFinished = new TaskCompletionSource(); + var testCompleted = new TaskCompletionSource(); + var readlockTask = Task.Run(async delegate + { + using (await this.asyncLock.ReadLockAsync()) + { + await lockAwaitFinished.SetAsync(); + await testCompleted.Task; + } + }); + + await lockAwaitFinished.Task; + + var cts = new CancellationTokenSource(); + + var awaitable = this.asyncLock.WriteLockAsync(cts.Token); + var awaiter = awaitable.GetAwaiter(); + Assert.False(awaiter.IsCompleted, "The lock should not be issued until read lock is issued."); + + cts.Cancel(); + awaiter.OnCompleted(delegate + { + try + { + awaiter.GetResult().Dispose(); + } + catch (OperationCanceledException) + { + } + + testCompleted.SetAsync(); + }); + + await readlockTask; + } + #endregion #region Completion tests From 40ce7c6060cfcb77d57f83b655367b5101909924 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 25 Jan 2018 18:02:30 -0800 Subject: [PATCH 4/4] Change to fix the AsyncReaderWriterLock race condition to handle CancellationToken. --- src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index 57ae59a73..c609925ae 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -2284,8 +2284,8 @@ public void OnCompleted(Action continuation) throw new NotSupportedException("Multiple continuations are not supported."); } - this.cancellationRegistration = this.cancellationToken.Register(CancellationResponseAction, this, useSynchronizationContext: false); this.lck.PendAwaiter(this); + this.cancellationRegistration = this.cancellationToken.Register(CancellationResponseAction, this, useSynchronizationContext: false); } ///