Merge pull request #715 from microsoft/dev/lifengl/writeLockRequestHang

Mitigate deadlock between AsyncReaderWriterLock and main thread

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
     /// </devnotes>
     public partial class AsyncReaderWriterLock : IDisposable
     {
+        /// <summary>
+        /// A time delay to check whether pending writer lock and reader locks forms a deadlock.
+        /// </summary>
+        private static readonly TimeSpan DefaultDeadlockCheckTimeout = TimeSpan.FromSeconds(3);
+
         /// <summary>
         /// The default SynchronizationContext to schedule work after issuing a lock.
         /// </summary>
@@ -46,6 +52,11 @@ public partial class AsyncReaderWriterLock : IDisposable
         /// </summary>
         private readonly object syncObject = new object();
 
+        /// <summary>
+        /// A JoinableTaskContext used to resolve dependencies between read locks to lead into deadlocks when there is a pending write lock.
+        /// </summary>
+        private readonly JoinableTaskContext? joinableTaskContext;
+
         /// <summary>
         /// A CallContext-local reference to the Awaiter that is on the top of the stack (most recently acquired).
         /// </summary>
@@ -138,11 +149,16 @@ public partial class AsyncReaderWriterLock : IDisposable
         /// </summary>
         private EventsHelper etw;
 
+        /// <summary>
+        /// A timer to recheck potential deadlock caused by pending writer locks.
+        /// </summary>
+        private Timer? pendingWriterLockDeadlockCheckTimer;
+
         /// <summary>
         /// Initializes a new instance of the <see cref="AsyncReaderWriterLock"/> class.
         /// </summary>
         public AsyncReaderWriterLock()
-            : this(captureDiagnostics: false)
+            : this(joinableTaskContext: null, captureDiagnostics: false)
         {
         }
 
@@ -153,8 +169,24 @@ public AsyncReaderWriterLock()
         /// <c>true</c> to spend additional resources capturing diagnostic details that can be used
         /// to analyze deadlocks or other issues.</param>
         public AsyncReaderWriterLock(bool captureDiagnostics)
+            : this(joinableTaskContext: null, captureDiagnostics)
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="AsyncReaderWriterLock"/> class.
+        /// </summary>
+        /// <param name="joinableTaskContext">
+        /// 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.
+        /// </param>
+        /// <param name="captureDiagnostics">
+        /// <c>true</c> to spend additional resources capturing diagnostic details that can be used
+        /// to analyze deadlocks or other issues.</param>
+        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; }
         }
 
+        /// <summary>
+        /// Gets a time delay to check whether pending writer lock and reader locks forms a deadlock.
+        /// </summary>
+        protected virtual TimeSpan DeadlockCheckTimeout => DefaultDeadlockCheckTimeout;
+
         /// <summary>
         /// Gets a value indicating whether the current thread is allowed to
         /// hold an active lock.
@@ -513,6 +550,18 @@ public void Dispose()
         /// <param name="disposing"><c>true</c> if <see cref="Dispose()"/> was called; <c>false</c> if the object is being finalized.</param>
         protected virtual void Dispose(bool disposing)
         {
+            if (disposing)
+            {
+                Timer? timerToDispose = null;
+
+                lock (this.syncObject)
+                {
+                    timerToDispose = this.pendingWriterLockDeadlockCheckTimer;
+                    this.pendingWriterLockDeadlockCheckTimer = null;
+                }
+
+                timerToDispose?.Dispose();
+            }
         }
 
         /// <summary>
@@ -725,6 +774,19 @@ private static bool HasAnyNestedLocks(Awaiter lck, HashSet<Awaiter> lockCollecti
             return false;
         }
 
+        private static void PendingWriterLockDeadlockWatchingCallback(object? state)
+        {
+            var readerWriterLock = (AsyncReaderWriterLock?)state;
+            Assumes.NotNull(readerWriterLock);
+
+            readerWriterLock.TryInvokeAllDependentReadersIfAppropriate();
+
+            lock (readerWriterLock.syncObject)
+            {
+                readerWriterLock.pendingWriterLockDeadlockCheckTimer?.Change((int)readerWriterLock.DeadlockCheckTimeout.TotalMilliseconds, -1);
+            }
+        }
+
         /// <summary>
         /// Throws an exception if called on an STA thread.
         /// </summary>
@@ -1001,9 +1063,14 @@ private void CheckSynchronizationContextAppropriateForLock(Awaiter? awaiter)
         /// The value is used to determine whether to reject it if <see cref="Complete"/> has already been called and this
         /// is a new top-level request.
         /// </param>
+        /// <param name="skipPendingWriteLockCheck">
+        /// 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 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.
+        /// </param>
         /// <returns>A value indicating whether the lock was issued.</returns>
         [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 +1097,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 +1288,7 @@ private Queue<Awaiter> 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 +1667,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<JoinableTask>? 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);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
         /// <summary>
         /// Issues a lock to the next queued upgradeable reader, if no upgradeable read or write locks are currently issued.
         /// </summary>
@@ -1641,6 +1739,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 +1821,35 @@ private void PendAwaiter(Awaiter awaiter)
                 {
                     Queue<Awaiter>? queue = this.GetLockQueue(awaiter.Kind);
                     queue.Enqueue(awaiter);
+
+                    if (awaiter.Kind == LockKind.Write)
+                    {
+                        this.StartPendingWriterDeadlockTimerIfNecessary();
+                    }
                 }
             }
         }
 
+        private void StartPendingWriterDeadlockTimerIfNecessary()
+        {
+            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, (int)this.DeadlockCheckTimeout.TotalMilliseconds, -1);
+            }
+        }
+
+        private void StopPendingWriterLockDeadlockWatching()
+        {
+            if (this.pendingWriterLockDeadlockCheckTimer != null)
+            {
+                this.pendingWriterLockDeadlockCheckTimer.Dispose();
+                this.pendingWriterLockDeadlockCheckTimer = null;
+            }
+        }
+
         /// <summary>
         /// Executes the lock receiver or releases the lock because the request for it was canceled before it was issued.
         /// </summary>
@@ -2180,6 +2308,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;
             }
 
             /// <summary>
@@ -2284,6 +2413,11 @@ internal bool IsReleased
                 }
             }
 
+            /// <summary>
+            /// 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.
+            /// </summary>
+            internal JoinableTask? AmbientJoinableTask { get; }
+
             /// <summary>
             /// Gets a value indicating whether the lock is active.
             /// </summary>

src/Microsoft.VisualStudio.Threading/AsyncReaderWriterResourceLock`2.cs

@@ -47,6 +47,21 @@ protected AsyncReaderWriterResourceLock(bool captureDiagnostics)
             this.helper = new Helper(this);
         }
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="AsyncReaderWriterResourceLock{TMoniker, TResource}"/> class.
+        /// </summary>
+        /// <param name="joinableTaskContext">
+        /// 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.
+        /// </param>
+        /// <param name="captureDiagnostics">
+        /// <c>true</c> to spend additional resources capturing diagnostic details that can be used
+        /// to analyze deadlocks or other issues.</param>
+        protected AsyncReaderWriterResourceLock(JoinableTaskContext? joinableTaskContext, bool captureDiagnostics)
+            : base(joinableTaskContext, captureDiagnostics)
+        {
+            this.helper = new Helper(this);
+        }
+
         /// <summary>
         /// Flags that modify default lock behavior.
         /// </summary>

src/Microsoft.VisualStudio.Threading/JoinableTaskDependencyGraph.cs

@@ -167,6 +167,58 @@ internal static void OnTaskCompleted(IJoinableTaskDependent taskItem)
             taskItem.GetJoinableTaskDependentData().OnTaskCompleted();
         }
 
+        /// <summary>
+        /// Get all tasks inside the candidate sets tasks, which are depended by one or more task in the source tasks list.
+        /// </summary>
+        /// <param name="sourceTasks">A collection of JoinableTasks represents source tasks.</param>
+        /// <param name="candidateTasks">A collection of JoinableTasks which represents candidates.</param>
+        /// <returns>A set of tasks matching the condition.</returns>
+        internal static HashSet<JoinableTask> GetDependentTasksFromCandidates(IEnumerable<JoinableTask> sourceTasks, IEnumerable<JoinableTask> candidateTasks)
+        {
+            Requires.NotNull(sourceTasks, nameof(sourceTasks));
+            Requires.NotNull(candidateTasks, nameof(candidateTasks));
+
+            var candidates = new HashSet<JoinableTask>(candidateTasks);
+            if (candidates.Count == 0)
+            {
+                return candidates;
+            }
+
+            var results = new HashSet<JoinableTask>();
+            var visited = new HashSet<IJoinableTaskDependent>();
+
+            var queue = new Queue<IJoinableTaskDependent>();
+            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;
+        }
+
         /// <summary>
         /// Preserve data for the JoinableTask dependency tree. It is holded inside either a <see cref="JoinableTask"/> or a <see cref="JoinableTaskCollection"/>.
         /// Do not call methods/properties directly anywhere out of <see cref="JoinableTaskDependencyGraph"/>.

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<TMoniker, TResource>.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void
+virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan

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<TMoniker, TResource>.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void
+virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan

src/Microsoft.VisualStudio.Threading/netstandard2.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<TMoniker, TResource>.AsyncReaderWriterResourceLock(Microsoft.VisualStudio.Threading.JoinableTaskContext? joinableTaskContext, bool captureDiagnostics) -> void
+virtual Microsoft.VisualStudio.Threading.AsyncReaderWriterLock.DeadlockCheckTimeout.get -> System.TimeSpan