From 94fae4eefd13d35801851fab1525ce2744ff2882 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 29 Oct 2020 12:01:36 -0700 Subject: [PATCH 1/8] Add a new unit test to reproduce the dead lock caused by a write lock request. --- .../AsyncReaderWriterLockTests.cs | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index b522acf8a..4b3cc22f3 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2171,6 +2171,67 @@ public async Task NewReadersWaitForWaitingWriters() newReaderLockHeld.Task); } + [Fact] + public async Task NoDeadLockByBlockingReaderLocks() + { + var firstReadLockHeld = new TaskCompletionSource(); + var writerWaitingForLock = new TaskCompletionSource(); + var newReaderWaiting = new TaskCompletionSource(); + var writeLockReleased = new TaskCompletionSource(); + + var computationTask = Task.Run(async delegate + { + await writerWaitingForLock.Task; + + this.Logger.WriteLine("About to wait for second read lock."); + using (await this.asyncLock.ReadLockAsync()) + { + this.Logger.WriteLine("Second read lock now held."); + await Task.Yield(); + this.Logger.WriteLine("Releasing first read lock."); + } + + this.Logger.WriteLine("Second read lock released."); + }); + + await Task.WhenAll( + Task.Run(async delegate + { + this.Logger.WriteLine("About to wait for first read lock."); + using (await this.asyncLock.ReadLockAsync()) + { + this.Logger.WriteLine("First read lock now held, and waiting a task requiring second read lock."); + await firstReadLockHeld.SetAsync(); + await computationTask; + + this.Logger.WriteLine("Releasing first read lock."); + } + }), + Task.Run(async delegate + { + await firstReadLockHeld.Task; + AsyncReaderWriterLock.Awaiter? writeAwaiter = this.asyncLock.WriteLockAsync().GetAwaiter(); + Assert.False(writeAwaiter.IsCompleted, "The writer should not be issued a lock while a read lock is held."); + this.Logger.WriteLine("Write lock in queue."); + writeAwaiter.OnCompleted(delegate + { + using (writeAwaiter.GetResult()) + { + this.Logger.WriteLine("Write lock issued."); + } + + writeLockReleased.SetResult(null); + }); + + await writerWaitingForLock.SetAsync(); + }), + computationTask, + firstReadLockHeld.Task, + writerWaitingForLock.Task, + newReaderWaiting.Task, + writeLockReleased.Task); + } + /// Verifies proper behavior when multiple read locks are held, and both read and write locks are in the queue, and a read lock is released. [Fact] public async Task ManyReadersBlockWriteAndSubsequentReadRequest() From 8893d9f9908d6e5b84edb1640ec1bc75b01debd6 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Tue, 10 Nov 2020 18:25:38 -0800 Subject: [PATCH 2/8] Initial dead lock fix. --- .../AsyncReaderWriterLock.cs | 139 +++++++++++++++++- .../AsyncReaderWriterResourceLock`2.cs | 15 ++ .../JoinableTaskDependencyGraph.cs | 52 +++++++ .../netstandard2.0/PublicAPI.Unshipped.txt | 3 + .../AsyncReaderWriterLockTests.cs | 45 ++++-- 5 files changed, 236 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index e5e1a5565..9d5d136e1 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -8,6 +8,7 @@ namespace Microsoft.VisualStudio.Threading using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; + using System.Linq; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -36,6 +37,11 @@ namespace Microsoft.VisualStudio.Threading /// public partial class AsyncReaderWriterLock : IDisposable { + /// + /// A time delay to check whether pending writer lock and reader locks forms a dead lock. + /// + private const int DefaultDeadLockCheckTimeOutInMilliseconds = 3 * 1000; + /// /// The default SynchronizationContext to schedule work after issuing a lock. /// @@ -46,6 +52,11 @@ public partial class AsyncReaderWriterLock : IDisposable /// private readonly object syncObject = new object(); + /// + /// A JoinableTaskContext used to resolve dependencies between read locks to lead into dead locks when there is a pending write lock. + /// + private readonly JoinableTaskContext? joinableTaskContext; + /// /// A CallContext-local reference to the Awaiter that is on the top of the stack (most recently acquired). /// @@ -138,11 +149,16 @@ public partial class AsyncReaderWriterLock : IDisposable /// private EventsHelper etw; + /// + /// A timer to recheck potential dead lock caused by pending writer locks. + /// + private Timer? pendingWriterLockDeadLockCheckTimer; + /// /// Initializes a new instance of the class. /// public AsyncReaderWriterLock() - : this(captureDiagnostics: false) + : this(joinableTaskContext: null, captureDiagnostics: false) { } @@ -153,8 +169,24 @@ public AsyncReaderWriterLock() /// true to spend additional resources capturing diagnostic details that can be used /// to analyze deadlocks or other issues. public AsyncReaderWriterLock(bool captureDiagnostics) + : this(joinableTaskContext: null, captureDiagnostics) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// + /// A JoinableTaskContext to help resolve dead locks caused by interdependency between top read lock tasks when there is a pending write lock blocking one of them. + /// + /// + /// true to spend additional resources capturing diagnostic details that can be used + /// to analyze deadlocks or other issues. + public AsyncReaderWriterLock(JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) { this.etw = new EventsHelper(this); + + this.joinableTaskContext = joinableTaskContext; this.captureDiagnostics = captureDiagnostics; } @@ -339,6 +371,11 @@ protected bool CaptureDiagnostics set { this.captureDiagnostics = value; } } + /// + /// Gets a time delay to check whether pending writer lock and reader locks forms a dead lock. + /// + protected virtual int DeadLockCheckTimeOutInMilliseconds => DefaultDeadLockCheckTimeOutInMilliseconds; + /// /// Gets a value indicating whether the current thread is allowed to /// hold an active lock. @@ -513,6 +550,14 @@ public void Dispose() /// true if was called; false if the object is being finalized. protected virtual void Dispose(bool disposing) { + if (disposing) + { + lock (this.syncObject) + { + this.pendingWriterLockDeadLockCheckTimer?.Dispose(); + this.pendingWriterLockDeadLockCheckTimer = null; + } + } } /// @@ -725,6 +770,20 @@ private static bool HasAnyNestedLocks(Awaiter lck, HashSet lockCollecti return false; } + private static void PendingWriterLockDeadLockWatchingCallback(object state) + { + var readerWriterLock = state as AsyncReaderWriterLock; + if (readerWriterLock != null) + { + readerWriterLock.TryInvokeAllDependentReadersIfAppropriate(); + + lock (readerWriterLock.syncObject) + { + readerWriterLock.pendingWriterLockDeadLockCheckTimer?.Change(readerWriterLock.DeadLockCheckTimeOutInMilliseconds, -1); + } + } + } + /// /// Throws an exception if called on an STA thread. /// @@ -1001,9 +1060,14 @@ private void CheckSynchronizationContextAppropriateForLock(Awaiter? awaiter) /// The value is used to determine whether to reject it if has already been called and this /// is a new top-level request. /// + /// + /// Normally, new reader locks are no longer issued when there is a pending writer lock to allow existing reader lock to complete. + /// However, that can lead dead locks, when tasks with issued lock depending on tasks requiring new read locks to complete. + /// When it is true, new reader locks will be issued even when there is a pending writer lock. + /// /// A value indicating whether the lock was issued. [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1502:AvoidExcessiveComplexity")] - private bool TryIssueLock(Awaiter awaiter, bool previouslyQueued) + private bool TryIssueLock(Awaiter awaiter, bool previouslyQueued, bool skipPendingWriteLockCheck = false) { lock (this.syncObject) { @@ -1030,7 +1094,7 @@ private bool TryIssueLock(Awaiter awaiter, bool previouslyQueued) switch (awaiter.Kind) { case LockKind.Read: - if (this.issuedWriteLocks.Count == 0 && this.waitingWriters.Count == 0) + if (this.issuedWriteLocks.Count == 0 && (skipPendingWriteLockCheck || this.waitingWriters.Count == 0)) { issued = true; } @@ -1221,7 +1285,7 @@ private Queue GetLockQueue(LockKind kind) private void IssueAndExecute(Awaiter awaiter) { EventsHelper.WaitStop(awaiter); - Assumes.True(this.TryIssueLock(awaiter, previouslyQueued: true)); + Assumes.True(this.TryIssueLock(awaiter, previouslyQueued: true, skipPendingWriteLockCheck: true)); Assumes.True(this.ExecuteOrHandleCancellation(awaiter, stillInQueue: false)); } @@ -1600,6 +1664,37 @@ private bool TryInvokeAllReadersIfAppropriate(bool searchAllWaiters) return invoked; } + private void TryInvokeAllDependentReadersIfAppropriate() + { + lock (this.syncObject) + { + if (this.issuedWriteLocks.Count == 0 && this.waitingWriters.Count > 0 && this.waitingReaders.Count > 0 && (this.issuedReadLocks.Count > 0 || this.issuedUpgradeableReadLocks.Count > 0)) + { + HashSet? dependentTasks = JoinableTaskDependencyGraph.GetDependentTasksFromCandidates( + this.issuedReadLocks.Concat(this.issuedUpgradeableReadLocks).Where(w => w.AmbientJoinableTask != null).Select(w => w.AmbientJoinableTask!), + this.waitingReaders.Where(w => w.AmbientJoinableTask != null).Select(w => w.AmbientJoinableTask!)); + + if (dependentTasks.Count > 0) + { + int pendingCount = this.waitingReaders.Count; + while (pendingCount-- != 0) + { + Awaiter? pendingReader = this.waitingReaders.Dequeue(); + JoinableTask? readerContext = pendingReader?.AmbientJoinableTask; + if (readerContext != null && dependentTasks.Contains(readerContext)) + { + this.IssueAndExecute(pendingReader!); + } + else + { + this.waitingReaders.Enqueue(pendingReader!); + } + } + } + } + } + } + /// /// Issues a lock to the next queued upgradeable reader, if no upgradeable read or write locks are currently issued. /// @@ -1641,6 +1736,11 @@ private bool TryInvokeOneWriterIfAppropriate(bool searchAllWaiters) if (this.waitingWriters.Count > 0) { Awaiter? pendingWriter = this.waitingWriters.Dequeue(); + if (this.waitingWriters.Count == 0) + { + this.StopPendingWriterLockDeadLockWatching(); + } + Assumes.True(pendingWriter.Kind == LockKind.Write); this.IssueAndExecute(pendingWriter); return true; @@ -1718,10 +1818,35 @@ private void PendAwaiter(Awaiter awaiter) { Queue? queue = this.GetLockQueue(awaiter.Kind); queue.Enqueue(awaiter); + + if (awaiter.Kind == LockKind.Write) + { + this.EnablePendingWriterLockDeadLockWatching(); + } } } } + private void EnablePendingWriterLockDeadLockWatching() + { + if (this.joinableTaskContext != null && + this.pendingWriterLockDeadLockCheckTimer == null && + this.waitingWriters.Count > 0 && + (this.issuedReadLocks.Count > 0 || this.issuedUpgradeableReadLocks.Count > 0)) + { + this.pendingWriterLockDeadLockCheckTimer = new Timer(PendingWriterLockDeadLockWatchingCallback, this, this.DeadLockCheckTimeOutInMilliseconds, -1); + } + } + + private void StopPendingWriterLockDeadLockWatching() + { + if (this.pendingWriterLockDeadLockCheckTimer != null && this.waitingWriters.Count == 0) + { + this.pendingWriterLockDeadLockCheckTimer.Dispose(); + this.pendingWriterLockDeadLockCheckTimer = null; + } + } + /// /// Executes the lock receiver or releases the lock because the request for it was canceled before it was issued. /// @@ -2180,6 +2305,7 @@ internal Awaiter(AsyncReaderWriterLock lck, LockKind kind, LockFlags options, Ca this.cancellationToken = cancellationToken; this.nestingLock = lck.GetFirstActiveSelfOrAncestor(lck.topAwaiter.Value); this.requestingStackTrace = lck.captureDiagnostics ? new StackTrace(2, true) : null; + this.AmbientJoinableTask = (this.nestingLock == null && this.kind != LockKind.Write) ? this.lck.joinableTaskContext?.AmbientTask : null; } /// @@ -2284,6 +2410,11 @@ internal bool IsReleased } } + /// + /// Gets the ambient JoinableTask when the lock is requested. This is used to resolve dead lock caused by issued read lock depending on new read lock requests blocked by pending write locks. + /// + internal JoinableTask? AmbientJoinableTask { get; } + /// /// Gets a value indicating whether the lock is active. /// diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterResourceLock`2.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterResourceLock`2.cs index 8677b868e..61813586b 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterResourceLock`2.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterResourceLock`2.cs @@ -47,6 +47,21 @@ protected AsyncReaderWriterResourceLock(bool captureDiagnostics) this.helper = new Helper(this); } + /// + /// Initializes a new instance of the class. + /// + /// + /// A JoinableTaskContext to help resolve dead locks caused by interdependency between top read lock tasks when there is a pending write lock blocking one of them. + /// + /// + /// true to spend additional resources capturing diagnostic details that can be used + /// to analyze deadlocks or other issues. + protected AsyncReaderWriterResourceLock(JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) + : base(joinableTaskContext, captureDiagnostics) + { + this.helper = new Helper(this); + } + /// /// Flags that modify default lock behavior. /// diff --git a/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs b/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs index eddebd4db..92a96b3e0 100644 --- a/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs +++ b/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs @@ -167,6 +167,58 @@ internal static void OnTaskCompleted(IJoinableTaskDependent taskItem) taskItem.GetJoinableTaskDependentData().OnTaskCompleted(); } + /// + /// Get all tasks inside the cadidate sets tasks, which are depended by one or more task in the source tasks list. + /// + /// A collection of JoinableTasks represents source tasks. + /// A collection of JoinableTasks which represents cadidates. + /// A set of tasks matching the condition. + internal static HashSet GetDependentTasksFromCandidates(IEnumerable sourceTasks, IEnumerable candidateTasks) + { + Requires.NotNull(sourceTasks, nameof(sourceTasks)); + Requires.NotNull(candidateTasks, nameof(candidateTasks)); + + var candidates = new HashSet(candidateTasks); + if (candidates.Count == 0) + { + return candidates; + } + + var results = new HashSet(); + var visited = new HashSet(); + + var queue = new Queue(); + foreach (JoinableTask? task in sourceTasks) + { + if (task != null && visited.Add(task)) + { + queue.Enqueue(task); + } + } + + while (queue.Count > 0) + { + IJoinableTaskDependent startDepenentNode = queue.Dequeue(); + if (candidates.Contains(startDepenentNode)) + { + results.Add((JoinableTask)startDepenentNode); + } + + lock (startDepenentNode.JoinableTaskContext.SyncContextLock) + { + foreach (IJoinableTaskDependent? dependentItem in JoinableTaskDependencyGraph.GetDirectDependentNodes(startDepenentNode)) + { + if (visited.Add(dependentItem)) + { + queue.Enqueue(dependentItem); + } + } + } + } + + return results; + } + /// /// Preserve data for the JoinableTask dependency tree. It is holded inside either a or a . /// Do not call methods/properties directly anywhere out of . diff --git a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt index 50283b574..46d3ffcf3 100644 --- a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt @@ -1,4 +1,6 @@ #nullable enable +Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void +Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaitable Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaitable.AggregateExceptionAwaitable(System.Threading.Tasks.Task! task, bool continueOnCapturedContext) -> void Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaitable.GetAwaiter() -> Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaiter @@ -9,3 +11,4 @@ Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaiter.IsCom Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaiter.OnCompleted(System.Action! continuation) -> void Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaiter.UnsafeOnCompleted(System.Action! continuation) -> void static Microsoft.VisualStudio.Threading.AwaitExtensions.ConfigureAwaitForAggregateException(this System.Threading.Tasks.Task! task, bool continueOnCapturedContext = true) -> Microsoft.VisualStudio.Threading.AwaitExtensions.AggregateExceptionAwaitable +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 4b3cc22f3..f6f220931 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2174,17 +2174,22 @@ public async Task NewReadersWaitForWaitingWriters() [Fact] public async Task NoDeadLockByBlockingReaderLocks() { + var taskContext = new JoinableTaskContext(); + var taskCollection = new JoinableTaskCollection(taskContext); + JoinableTaskFactory? taskFactory = taskContext.CreateFactory(taskCollection); + + var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + var firstReadLockHeld = new TaskCompletionSource(); var writerWaitingForLock = new TaskCompletionSource(); - var newReaderWaiting = new TaskCompletionSource(); var writeLockReleased = new TaskCompletionSource(); - var computationTask = Task.Run(async delegate + JoinableTask computationTask = taskFactory.RunAsync(async delegate { await writerWaitingForLock.Task; this.Logger.WriteLine("About to wait for second read lock."); - using (await this.asyncLock.ReadLockAsync()) + using (await lockService.ReadLockAsync()) { this.Logger.WriteLine("Second read lock now held."); await Task.Yield(); @@ -2195,22 +2200,25 @@ public async Task NoDeadLockByBlockingReaderLocks() }); await Task.WhenAll( - Task.Run(async delegate + Task.Run(delegate { - this.Logger.WriteLine("About to wait for first read lock."); - using (await this.asyncLock.ReadLockAsync()) + return taskFactory.RunAsync(async delegate { - this.Logger.WriteLine("First read lock now held, and waiting a task requiring second read lock."); - await firstReadLockHeld.SetAsync(); - await computationTask; + this.Logger.WriteLine("About to wait for first read lock."); + using (await lockService.ReadLockAsync()) + { + this.Logger.WriteLine("First read lock now held, and waiting a task requiring second read lock."); + await firstReadLockHeld.SetAsync(); + await computationTask; - this.Logger.WriteLine("Releasing first read lock."); - } + this.Logger.WriteLine("Releasing first read lock."); + } + }); }), Task.Run(async delegate { await firstReadLockHeld.Task; - AsyncReaderWriterLock.Awaiter? writeAwaiter = this.asyncLock.WriteLockAsync().GetAwaiter(); + AsyncReaderWriterLock.Awaiter? writeAwaiter = lockService.WriteLockAsync().GetAwaiter(); Assert.False(writeAwaiter.IsCompleted, "The writer should not be issued a lock while a read lock is held."); this.Logger.WriteLine("Write lock in queue."); writeAwaiter.OnCompleted(delegate @@ -2225,10 +2233,9 @@ public async Task NoDeadLockByBlockingReaderLocks() await writerWaitingForLock.SetAsync(); }), - computationTask, + computationTask.Task, firstReadLockHeld.Task, writerWaitingForLock.Task, - newReaderWaiting.Task, writeLockReleased.Task); } @@ -5027,6 +5034,16 @@ protected override async Task OnBeforeExclusiveLockReleasedAsync() } } + private class ReaderWriterLockWithFastDeadLockCheck : AsyncReaderWriterLock + { + public ReaderWriterLockWithFastDeadLockCheck(JoinableTaskContext joinableTaskContext) + : base(joinableTaskContext) + { + } + + protected override int DeadLockCheckTimeOutInMilliseconds => 50; + } + private class AsyncReaderWriterLockWithSpecialScheduler : AsyncReaderWriterLock { private readonly SpecialTaskScheduler scheduler; From 921b1a033b4e85c3a370ce2bf446da2e49567aa7 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 12 Nov 2020 11:15:40 -0800 Subject: [PATCH 3/8] Fix build warnings. --- src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs | 2 +- .../net472/PublicAPI.Unshipped.txt | 3 +++ .../netcoreapp3.0/PublicAPI.Unshipped.txt | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index 9d5d136e1..7b6690f8a 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -770,7 +770,7 @@ private static bool HasAnyNestedLocks(Awaiter lck, HashSet lockCollecti return false; } - private static void PendingWriterLockDeadLockWatchingCallback(object state) + private static void PendingWriterLockDeadLockWatchingCallback(object? state) { var readerWriterLock = state as AsyncReaderWriterLock; if (readerWriterLock != null) diff --git a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt index e69de29bb..b7d7fb880 100644 --- a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt @@ -0,0 +1,3 @@ +Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void +Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int diff --git a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt index e69de29bb..b7d7fb880 100644 --- a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt @@ -0,0 +1,3 @@ +Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void +Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int From 95234d07b99c325c07aa0ec931327ae574e14ae4 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Wed, 18 Nov 2020 15:21:25 -0800 Subject: [PATCH 4/8] Fix problems raised in the code review. --- .../AsyncReaderWriterLock.cs | 71 ++++++++++--------- .../JoinableTaskDependencyGraph.cs | 6 +- .../net472/PublicAPI.Unshipped.txt | 2 +- .../netcoreapp3.0/PublicAPI.Unshipped.txt | 2 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 2 +- .../AsyncReaderWriterLockTests.cs | 2 +- 6 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index 7b6690f8a..57209f41d 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -38,9 +38,9 @@ namespace Microsoft.VisualStudio.Threading public partial class AsyncReaderWriterLock : IDisposable { /// - /// A time delay to check whether pending writer lock and reader locks forms a dead lock. + /// A time delay to check whether pending writer lock and reader locks forms a deadlock. /// - private const int DefaultDeadLockCheckTimeOutInMilliseconds = 3 * 1000; + private static readonly TimeSpan DefaultDeadlockCheckTimeOut = new TimeSpan(hours: 0, minutes: 0, seconds: 3); /// /// The default SynchronizationContext to schedule work after issuing a lock. @@ -53,7 +53,7 @@ public partial class AsyncReaderWriterLock : IDisposable private readonly object syncObject = new object(); /// - /// A JoinableTaskContext used to resolve dependencies between read locks to lead into dead locks when there is a pending write lock. + /// A JoinableTaskContext used to resolve dependencies between read locks to lead into deadlocks when there is a pending write lock. /// private readonly JoinableTaskContext? joinableTaskContext; @@ -150,9 +150,9 @@ public partial class AsyncReaderWriterLock : IDisposable private EventsHelper etw; /// - /// A timer to recheck potential dead lock caused by pending writer locks. + /// A timer to recheck potential deadlock caused by pending writer locks. /// - private Timer? pendingWriterLockDeadLockCheckTimer; + private Timer? pendingWriterLockDeadlockCheckTimer; /// /// Initializes a new instance of the class. @@ -177,7 +177,7 @@ public AsyncReaderWriterLock(bool captureDiagnostics) /// Initializes a new instance of the class. /// /// - /// A JoinableTaskContext to help resolve dead locks caused by interdependency between top read lock tasks when there is a pending write lock blocking one of them. + /// A JoinableTaskContext to help resolve deadlocks caused by interdependency between top read lock tasks when there is a pending write lock blocking one of them. /// /// /// true to spend additional resources capturing diagnostic details that can be used @@ -372,9 +372,9 @@ protected bool CaptureDiagnostics } /// - /// Gets a time delay to check whether pending writer lock and reader locks forms a dead lock. + /// Gets a time delay to check whether pending writer lock and reader locks forms a deadlock. /// - protected virtual int DeadLockCheckTimeOutInMilliseconds => DefaultDeadLockCheckTimeOutInMilliseconds; + protected virtual TimeSpan DeadlockCheckTimeOut => DefaultDeadlockCheckTimeOut; /// /// Gets a value indicating whether the current thread is allowed to @@ -552,11 +552,15 @@ protected virtual void Dispose(bool disposing) { if (disposing) { + Timer? timerToDispose = null; + lock (this.syncObject) { - this.pendingWriterLockDeadLockCheckTimer?.Dispose(); - this.pendingWriterLockDeadLockCheckTimer = null; + timerToDispose = this.pendingWriterLockDeadlockCheckTimer; + this.pendingWriterLockDeadlockCheckTimer = null; } + + timerToDispose?.Dispose(); } } @@ -770,17 +774,16 @@ private static bool HasAnyNestedLocks(Awaiter lck, HashSet lockCollecti return false; } - private static void PendingWriterLockDeadLockWatchingCallback(object? state) + private static void PendingWriterLockDeadlockWatchingCallback(object? state) { - var readerWriterLock = state as AsyncReaderWriterLock; - if (readerWriterLock != null) - { - readerWriterLock.TryInvokeAllDependentReadersIfAppropriate(); + var readerWriterLock = (AsyncReaderWriterLock?)state; + Assumes.NotNull(readerWriterLock); - lock (readerWriterLock.syncObject) - { - readerWriterLock.pendingWriterLockDeadLockCheckTimer?.Change(readerWriterLock.DeadLockCheckTimeOutInMilliseconds, -1); - } + readerWriterLock.TryInvokeAllDependentReadersIfAppropriate(); + + lock (readerWriterLock.syncObject) + { + readerWriterLock.pendingWriterLockDeadlockCheckTimer?.Change((int)readerWriterLock.DeadlockCheckTimeOut.TotalMilliseconds, -1); } } @@ -1062,7 +1065,7 @@ private void CheckSynchronizationContextAppropriateForLock(Awaiter? awaiter) /// /// /// Normally, new reader locks are no longer issued when there is a pending writer lock to allow existing reader lock to complete. - /// However, that can lead dead locks, when tasks with issued lock depending on tasks requiring new read locks to complete. + /// However, that can lead deadlocks, when tasks with issued lock depending on tasks requiring new read locks to complete. /// When it is true, new reader locks will be issued even when there is a pending writer lock. /// /// A value indicating whether the lock was issued. @@ -1679,15 +1682,15 @@ private void TryInvokeAllDependentReadersIfAppropriate() int pendingCount = this.waitingReaders.Count; while (pendingCount-- != 0) { - Awaiter? pendingReader = this.waitingReaders.Dequeue(); - JoinableTask? readerContext = pendingReader?.AmbientJoinableTask; + Awaiter pendingReader = this.waitingReaders.Dequeue(); + JoinableTask? readerContext = pendingReader.AmbientJoinableTask; if (readerContext != null && dependentTasks.Contains(readerContext)) { - this.IssueAndExecute(pendingReader!); + this.IssueAndExecute(pendingReader); } else { - this.waitingReaders.Enqueue(pendingReader!); + this.waitingReaders.Enqueue(pendingReader); } } } @@ -1738,7 +1741,7 @@ private bool TryInvokeOneWriterIfAppropriate(bool searchAllWaiters) Awaiter? pendingWriter = this.waitingWriters.Dequeue(); if (this.waitingWriters.Count == 0) { - this.StopPendingWriterLockDeadLockWatching(); + this.StopPendingWriterLockDeadlockWatching(); } Assumes.True(pendingWriter.Kind == LockKind.Write); @@ -1821,29 +1824,29 @@ private void PendAwaiter(Awaiter awaiter) if (awaiter.Kind == LockKind.Write) { - this.EnablePendingWriterLockDeadLockWatching(); + this.StartPendingWriterDeadlockTimerIfNecessary(); } } } } - private void EnablePendingWriterLockDeadLockWatching() + private void StartPendingWriterDeadlockTimerIfNecessary() { if (this.joinableTaskContext != null && - this.pendingWriterLockDeadLockCheckTimer == null && + this.pendingWriterLockDeadlockCheckTimer == null && this.waitingWriters.Count > 0 && (this.issuedReadLocks.Count > 0 || this.issuedUpgradeableReadLocks.Count > 0)) { - this.pendingWriterLockDeadLockCheckTimer = new Timer(PendingWriterLockDeadLockWatchingCallback, this, this.DeadLockCheckTimeOutInMilliseconds, -1); + this.pendingWriterLockDeadlockCheckTimer = new Timer(PendingWriterLockDeadlockWatchingCallback, this, (int)this.DeadlockCheckTimeOut.TotalMilliseconds, -1); } } - private void StopPendingWriterLockDeadLockWatching() + private void StopPendingWriterLockDeadlockWatching() { - if (this.pendingWriterLockDeadLockCheckTimer != null && this.waitingWriters.Count == 0) + if (this.pendingWriterLockDeadlockCheckTimer != null) { - this.pendingWriterLockDeadLockCheckTimer.Dispose(); - this.pendingWriterLockDeadLockCheckTimer = null; + this.pendingWriterLockDeadlockCheckTimer.Dispose(); + this.pendingWriterLockDeadlockCheckTimer = null; } } @@ -2411,7 +2414,7 @@ internal bool IsReleased } /// - /// Gets the ambient JoinableTask when the lock is requested. This is used to resolve dead lock caused by issued read lock depending on new read lock requests blocked by pending write locks. + /// Gets the ambient JoinableTask when the lock is requested. This is used to resolve deadlock caused by issued read lock depending on new read lock requests blocked by pending write locks. /// internal JoinableTask? AmbientJoinableTask { get; } diff --git a/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs b/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs index 92a96b3e0..895e15b0f 100644 --- a/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs +++ b/src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs @@ -168,10 +168,10 @@ internal static void OnTaskCompleted(IJoinableTaskDependent taskItem) } /// - /// Get all tasks inside the cadidate sets tasks, which are depended by one or more task in the source tasks list. + /// Get all tasks inside the candidate sets tasks, which are depended by one or more task in the source tasks list. /// /// A collection of JoinableTasks represents source tasks. - /// A collection of JoinableTasks which represents cadidates. + /// A collection of JoinableTasks which represents candidates. /// A set of tasks matching the condition. internal static HashSet GetDependentTasksFromCandidates(IEnumerable sourceTasks, IEnumerable candidateTasks) { @@ -188,7 +188,7 @@ internal static HashSet GetDependentTasksFromCandidates(IEnumerabl var visited = new HashSet(); var queue = new Queue(); - foreach (JoinableTask? task in sourceTasks) + foreach (JoinableTask task in sourceTasks) { if (task != null && visited.Add(task)) { diff --git a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt index b7d7fb880..58cf85726 100644 --- a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan diff --git a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt index b7d7fb880..58cf85726 100644 --- a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan diff --git a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt index b7d7fb880..58cf85726 100644 --- a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadLockCheckTimeOutInMilliseconds.get -> int +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index f6f220931..da53ef0f5 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -5041,7 +5041,7 @@ public ReaderWriterLockWithFastDeadLockCheck(JoinableTaskContext joinableTaskCon { } - protected override int DeadLockCheckTimeOutInMilliseconds => 50; + protected override TimeSpan DeadlockCheckTimeOut { get; } = new TimeSpan(days: 0, hours: 0, minutes: 0, seconds: 0, milliseconds: 50); } private class AsyncReaderWriterLockWithSpecialScheduler : AsyncReaderWriterLock From 7ba862d1974ecd055e4ead5f8d2df36798ca239c Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Wed, 18 Nov 2020 18:56:24 -0800 Subject: [PATCH 5/8] Add more unit tests. --- .../AsyncReaderWriterLockTests.cs | 177 +++++++++++++++++- 1 file changed, 174 insertions(+), 3 deletions(-) diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index da53ef0f5..6d34ec1e9 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2172,11 +2172,11 @@ public async Task NewReadersWaitForWaitingWriters() } [Fact] - public async Task NoDeadLockByBlockingReaderLocks() + public async Task NoDeadlockByBlockingReaderLocks() { var taskContext = new JoinableTaskContext(); var taskCollection = new JoinableTaskCollection(taskContext); - JoinableTaskFactory? taskFactory = taskContext.CreateFactory(taskCollection); + JoinableTaskFactory taskFactory = taskContext.CreateFactory(taskCollection); var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); @@ -2193,7 +2193,7 @@ public async Task NoDeadLockByBlockingReaderLocks() { this.Logger.WriteLine("Second read lock now held."); await Task.Yield(); - this.Logger.WriteLine("Releasing first read lock."); + this.Logger.WriteLine("Releasing second read lock."); } this.Logger.WriteLine("Second read lock released."); @@ -2239,6 +2239,177 @@ public async Task NoDeadLockByBlockingReaderLocks() writeLockReleased.Task); } + [Fact] + public async Task NoDeadlockByBlockingUpgradeableReaderLocks() + { + var taskContext = new JoinableTaskContext(); + var taskCollection = new JoinableTaskCollection(taskContext); + JoinableTaskFactory taskFactory = taskContext.CreateFactory(taskCollection); + + var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + + var firstReadLockHeld = new TaskCompletionSource(); + var writerWaitingForLock = new TaskCompletionSource(); + var writeLockReleased = new TaskCompletionSource(); + + JoinableTask computationTask = taskFactory.RunAsync(async delegate + { + await writerWaitingForLock.Task; + + this.Logger.WriteLine("About to wait for second read lock."); + using (await lockService.ReadLockAsync()) + { + this.Logger.WriteLine("Second read lock now held."); + await Task.Yield(); + this.Logger.WriteLine("Releasing second read lock."); + } + + this.Logger.WriteLine("Second read lock released."); + }); + + await Task.WhenAll( + Task.Run(delegate + { + return taskFactory.RunAsync(async delegate + { + this.Logger.WriteLine("About to wait for first read lock."); + using (await lockService.UpgradeableReadLockAsync()) + { + this.Logger.WriteLine("First upgradable read lock now held, and waiting a task requiring second read lock."); + await firstReadLockHeld.SetAsync(); + await computationTask; + + this.Logger.WriteLine("Releasing upgradable read lock."); + } + }); + }), + Task.Run(async delegate + { + await firstReadLockHeld.Task; + AsyncReaderWriterLock.Awaiter? writeAwaiter = lockService.WriteLockAsync().GetAwaiter(); + Assert.False(writeAwaiter.IsCompleted, "The writer should not be issued a lock while a read lock is held."); + this.Logger.WriteLine("Write lock in queue."); + writeAwaiter.OnCompleted(delegate + { + using (writeAwaiter.GetResult()) + { + this.Logger.WriteLine("Write lock issued."); + } + + writeLockReleased.SetResult(null); + }); + + await writerWaitingForLock.SetAsync(); + }), + firstReadLockHeld.Task, + writerWaitingForLock.Task, + writeLockReleased.Task); + + await computationTask.Task; + } + + [Fact] + public async Task NoDeadlockByBlockingSequenceReaderLocks() + { + var taskContext = new JoinableTaskContext(); + var taskCollection = new JoinableTaskCollection(taskContext); + JoinableTaskFactory? taskFactory = taskContext.CreateFactory(taskCollection); + + var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + + var firstLockHeld = new TaskCompletionSource(); + var writerWaitingForLock = new TaskCompletionSource(); + var writeLockReleased = new TaskCompletionSource(); + + var computationTasks = new JoinableTask[4]; + + for (int i = 0; i < computationTasks.Length; i++) + { + computationTasks[i] = CreateReaderLockTask(taskFactory, lockService, writerWaitingForLock.Task, i, i > 0 ? computationTasks[i - 1] : null); + } + + JoinableTask unrelatedTask = taskFactory.RunAsync(async delegate + { + await writerWaitingForLock.Task; + + this.Logger.WriteLine("About to wait for unrelated read lock."); + using (await lockService.ReadLockAsync()) + { + this.Logger.WriteLine("unrelated read lock now held."); + await Task.Yield(); + this.Logger.WriteLine("Releasing unrelated read lock."); + } + + this.Logger.WriteLine("unrelated read lock released."); + }); + + await Task.WhenAll( + Task.Run(delegate + { + return taskFactory.RunAsync(async delegate + { + this.Logger.WriteLine("About to wait for upgradeable read lock."); + using (await lockService.UpgradeableReadLockAsync()) + { + this.Logger.WriteLine("upgradeable read lock now held, and waiting a task requiring related read lock."); + await firstLockHeld.SetAsync(); + await computationTasks[computationTasks.Length - 1]; + + this.Logger.WriteLine("Releasing upgradeable read lock."); + } + }); + }), + Task.Run(async delegate + { + await firstLockHeld.Task; + AsyncReaderWriterLock.Awaiter? writeAwaiter = lockService.WriteLockAsync().GetAwaiter(); + Assert.False(writeAwaiter.IsCompleted, "The writer should not be issued a lock while a read lock is held."); + this.Logger.WriteLine("Write lock in queue."); + writeAwaiter.OnCompleted(delegate + { + using (writeAwaiter.GetResult()) + { + this.Logger.WriteLine("Write lock issued."); + } + + Assert.False(unrelatedTask.IsCompleted, "Unrelated reader lock should not be issued."); + + writeLockReleased.SetResult(null); + }); + + await writerWaitingForLock.SetAsync(); + }), + firstLockHeld.Task, + writerWaitingForLock.Task, + unrelatedTask.Task, + writeLockReleased.Task); + + await Task.WhenAll(computationTasks.Select(t => t.Task)); + + JoinableTask CreateReaderLockTask(JoinableTaskFactory taskFactory, AsyncReaderWriterLock lockService, Task initTask, int sequence, JoinableTask? previousTask) + { + return taskFactory.RunAsync(async delegate + { + await initTask; + + this.Logger.WriteLine($"About to wait for read lock {sequence}."); + using (await lockService.ReadLockAsync()) + { + this.Logger.WriteLine($"Related read lock {sequence} now held."); + await Task.Yield(); + this.Logger.WriteLine($"Releasing related read lock {sequence}."); + } + + if (previousTask != null) + { + await previousTask; + } + + this.Logger.WriteLine($"Read lock {sequence} released."); + }); + } + } + /// Verifies proper behavior when multiple read locks are held, and both read and write locks are in the queue, and a read lock is released. [Fact] public async Task ManyReadersBlockWriteAndSubsequentReadRequest() From f226d686e4e38f171d109065f03aaa4f1a4f0d7c Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Thu, 19 Nov 2020 11:10:14 -0800 Subject: [PATCH 6/8] Fix assertion in a wrong location in the unit test. --- .../AsyncReaderWriterLockTests.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 6d34ec1e9..c718b9f67 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2348,14 +2348,14 @@ public async Task NoDeadlockByBlockingSequenceReaderLocks() { return taskFactory.RunAsync(async delegate { - this.Logger.WriteLine("About to wait for upgradeable read lock."); - using (await lockService.UpgradeableReadLockAsync()) + this.Logger.WriteLine("About to wait for first read lock."); + using (await lockService.ReadLockAsync()) { - this.Logger.WriteLine("upgradeable read lock now held, and waiting a task requiring related read lock."); + this.Logger.WriteLine("first read lock now held, and waiting a task requiring related read lock."); await firstLockHeld.SetAsync(); await computationTasks[computationTasks.Length - 1]; - this.Logger.WriteLine("Releasing upgradeable read lock."); + this.Logger.WriteLine("Releasing first read lock."); } }); }), @@ -2370,10 +2370,9 @@ public async Task NoDeadlockByBlockingSequenceReaderLocks() using (writeAwaiter.GetResult()) { this.Logger.WriteLine("Write lock issued."); + Assert.False(unrelatedTask.IsCompleted, "Unrelated reader lock should not be issued."); } - Assert.False(unrelatedTask.IsCompleted, "Unrelated reader lock should not be issued."); - writeLockReleased.SetResult(null); }); From 4752ef9bbfc2914d955c18b8db4ce9c3c798693d Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Fri, 20 Nov 2020 12:00:48 -0800 Subject: [PATCH 7/8] Use better methods to create TimeSpan. --- src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs | 2 +- .../AsyncReaderWriterLockTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index 57209f41d..7af2212d8 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -40,7 +40,7 @@ public partial class AsyncReaderWriterLock : IDisposable /// /// A time delay to check whether pending writer lock and reader locks forms a deadlock. /// - private static readonly TimeSpan DefaultDeadlockCheckTimeOut = new TimeSpan(hours: 0, minutes: 0, seconds: 3); + private static readonly TimeSpan DefaultDeadlockCheckTimeOut = TimeSpan.FromSeconds(3); /// /// The default SynchronizationContext to schedule work after issuing a lock. diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index c718b9f67..487de9e43 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -5211,7 +5211,7 @@ public ReaderWriterLockWithFastDeadLockCheck(JoinableTaskContext joinableTaskCon { } - protected override TimeSpan DeadlockCheckTimeOut { get; } = new TimeSpan(days: 0, hours: 0, minutes: 0, seconds: 0, milliseconds: 50); + protected override TimeSpan DeadlockCheckTimeOut { get; } = TimeSpan.FromMilliseconds(50); } private class AsyncReaderWriterLockWithSpecialScheduler : AsyncReaderWriterLock From 654ee33702024fa48165fd5c963784faa4492cd4 Mon Sep 17 00:00:00 2001 From: Lifeng Lu Date: Fri, 20 Nov 2020 18:28:59 -0800 Subject: [PATCH 8/8] Update some spelling. --- .../AsyncReaderWriterLock.cs | 8 ++++---- .../net472/PublicAPI.Unshipped.txt | 2 +- .../netcoreapp3.0/PublicAPI.Unshipped.txt | 2 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 2 +- .../AsyncReaderWriterLockTests.cs | 12 ++++++------ 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs index 7af2212d8..d0de1f4a9 100644 --- a/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs +++ b/src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs @@ -40,7 +40,7 @@ public partial class AsyncReaderWriterLock : IDisposable /// /// A time delay to check whether pending writer lock and reader locks forms a deadlock. /// - private static readonly TimeSpan DefaultDeadlockCheckTimeOut = TimeSpan.FromSeconds(3); + private static readonly TimeSpan DefaultDeadlockCheckTimeout = TimeSpan.FromSeconds(3); /// /// The default SynchronizationContext to schedule work after issuing a lock. @@ -374,7 +374,7 @@ protected bool CaptureDiagnostics /// /// Gets a time delay to check whether pending writer lock and reader locks forms a deadlock. /// - protected virtual TimeSpan DeadlockCheckTimeOut => DefaultDeadlockCheckTimeOut; + protected virtual TimeSpan DeadlockCheckTimeout => DefaultDeadlockCheckTimeout; /// /// Gets a value indicating whether the current thread is allowed to @@ -783,7 +783,7 @@ private static void PendingWriterLockDeadlockWatchingCallback(object? state) lock (readerWriterLock.syncObject) { - readerWriterLock.pendingWriterLockDeadlockCheckTimer?.Change((int)readerWriterLock.DeadlockCheckTimeOut.TotalMilliseconds, -1); + readerWriterLock.pendingWriterLockDeadlockCheckTimer?.Change((int)readerWriterLock.DeadlockCheckTimeout.TotalMilliseconds, -1); } } @@ -1837,7 +1837,7 @@ private void StartPendingWriterDeadlockTimerIfNecessary() this.waitingWriters.Count > 0 && (this.issuedReadLocks.Count > 0 || this.issuedUpgradeableReadLocks.Count > 0)) { - this.pendingWriterLockDeadlockCheckTimer = new Timer(PendingWriterLockDeadlockWatchingCallback, this, (int)this.DeadlockCheckTimeOut.TotalMilliseconds, -1); + this.pendingWriterLockDeadlockCheckTimer = new Timer(PendingWriterLockDeadlockWatchingCallback, this, (int)this.DeadlockCheckTimeout.TotalMilliseconds, -1); } } diff --git a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt index 58cf85726..f6cf3e6b6 100644 --- a/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/net472/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan diff --git a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt index 58cf85726..f6cf3e6b6 100644 --- a/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netcoreapp3.0/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan diff --git a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt index 58cf85726..f6cf3e6b6 100644 --- a/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/Microsoft.VisualStudio.Threading/netstandard2.0/PublicAPI.Unshipped.txt @@ -1,3 +1,3 @@ Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.AsyncReaderWriterLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics = false) -> void Microsoft.VisualStudio.Threading.AsyncReaderWriterResourceLock.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void -virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeOut.get -> System.TimeSpan +virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan diff --git a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs index 487de9e43..3163a3edb 100644 --- a/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs +++ b/test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs @@ -2178,7 +2178,7 @@ public async Task NoDeadlockByBlockingReaderLocks() var taskCollection = new JoinableTaskCollection(taskContext); JoinableTaskFactory taskFactory = taskContext.CreateFactory(taskCollection); - var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + var lockService = new ReaderWriterLockWithFastDeadlockCheck(taskContext); var firstReadLockHeld = new TaskCompletionSource(); var writerWaitingForLock = new TaskCompletionSource(); @@ -2246,7 +2246,7 @@ public async Task NoDeadlockByBlockingUpgradeableReaderLocks() var taskCollection = new JoinableTaskCollection(taskContext); JoinableTaskFactory taskFactory = taskContext.CreateFactory(taskCollection); - var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + var lockService = new ReaderWriterLockWithFastDeadlockCheck(taskContext); var firstReadLockHeld = new TaskCompletionSource(); var writerWaitingForLock = new TaskCompletionSource(); @@ -2315,7 +2315,7 @@ public async Task NoDeadlockByBlockingSequenceReaderLocks() var taskCollection = new JoinableTaskCollection(taskContext); JoinableTaskFactory? taskFactory = taskContext.CreateFactory(taskCollection); - var lockService = new ReaderWriterLockWithFastDeadLockCheck(taskContext); + var lockService = new ReaderWriterLockWithFastDeadlockCheck(taskContext); var firstLockHeld = new TaskCompletionSource(); var writerWaitingForLock = new TaskCompletionSource(); @@ -5204,14 +5204,14 @@ protected override async Task OnBeforeExclusiveLockReleasedAsync() } } - private class ReaderWriterLockWithFastDeadLockCheck : AsyncReaderWriterLock + private class ReaderWriterLockWithFastDeadlockCheck : AsyncReaderWriterLock { - public ReaderWriterLockWithFastDeadLockCheck(JoinableTaskContext joinableTaskContext) + public ReaderWriterLockWithFastDeadlockCheck(JoinableTaskContext joinableTaskContext) : base(joinableTaskContext) { } - protected override TimeSpan DeadlockCheckTimeOut { get; } = TimeSpan.FromMilliseconds(50); + protected override TimeSpan DeadlockCheckTimeout { get; } = TimeSpan.FromMilliseconds(50); } private class AsyncReaderWriterLockWithSpecialScheduler : AsyncReaderWriterLock