Swap the order of lock taken

Move the Cancel() out of OnReparseRequest and SuspendParser method and into between the locks for both ParseAll and ParseInternal methods.
rubberduck-vba · Jun 20, 2018 · 21160f9 · 21160f9
1 parent 266ae63
commit 21160f9
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 18 deletions.
diff --git a/Rubberduck.Parsing/VBA/ParseCoordinator.cs b/Rubberduck.Parsing/VBA/ParseCoordinator.cs
@@ -138,7 +138,9 @@ public void SuspendRequested(object sender, RubberduckStatusSuspendParserEventAr
                 _parserStateManager.SetStatusAndFireStateChanged(e.Requestor, ParserState.Busy,
                     CancellationToken.None);
                 e.BusyAction.Invoke();
-
+            }
+            finally
+            {
                 lock (_suspendStackSyncObject)
                 {
                     _isSuspended = false;
@@ -148,9 +150,7 @@ public void SuspendRequested(object sender, RubberduckStatusSuspendParserEventAr
                         parseRequestor = lastRequestor;
                     }
                 }
-            }
-            finally
-            {
+
                 if (_parsingSuspendLock.IsWriteLockHeld)
                 {
                     _parsingSuspendLock.ExitWriteLock();
@@ -159,7 +159,6 @@ public void SuspendRequested(object sender, RubberduckStatusSuspendParserEventAr
 
             if (parseRequestor != null)
             {
-                Cancel();
                 BeginParse(parseRequestor, _currentCancellationTokenSource.Token);
             }
             else if (originalStatus != ParserState.Ready)
@@ -231,11 +230,16 @@ private void ParseInternal(CancellationToken token)
             var lockTaken = false;
             try
             {
-                Monitor.Enter(_parsingRunSyncObject, ref lockTaken);
                 if (!_parsingSuspendLock.IsWriteLockHeld)
                 {
                     _parsingSuspendLock.EnterReadLock();
                 }
+                lock (_cancellationSyncObject)
+                {
+                    Cancel();
+                    token = _currentCancellationTokenSource.Token;
+                }
+                Monitor.Enter(_parsingRunSyncObject, ref lockTaken);
                 ParseAllInternal(this, token);
             }
             catch (OperationCanceledException)
@@ -244,14 +248,14 @@ private void ParseInternal(CancellationToken token)
             }
             finally
             {
-                if (_parsingSuspendLock.IsReadLockHeld)
-                {
-                    _parsingSuspendLock.ExitReadLock();
-                }
                 if (lockTaken)
                 {
                     Monitor.Exit(_parsingRunSyncObject);
                 }
+                if (_parsingSuspendLock.IsReadLockHeld)
+                {
+                    _parsingSuspendLock.ExitReadLock();
+                }
             }
         }
 
@@ -404,12 +408,17 @@ private void ParseAll(object requestor, CancellationToken token)
             var lockTaken = false;
             try
             {
-                Monitor.Enter(_parsingRunSyncObject, ref lockTaken);
                 if (!_parsingSuspendLock.IsWriteLockHeld)
                 {
                     _parsingSuspendLock.EnterReadLock();
                 }
-
+                lock (_cancellationSyncObject)
+                {
+                    Cancel();
+                    token = _currentCancellationTokenSource.Token;
+                }
+                Monitor.Enter(_parsingRunSyncObject, ref lockTaken);
+
                 watch = Stopwatch.StartNew();
                 Logger.Debug("Parsing run started. (thread {0}).", Thread.CurrentThread.ManagedThreadId);
 
@@ -431,11 +440,11 @@ private void ParseAll(object requestor, CancellationToken token)
             finally
             {
                 if (watch != null && watch.IsRunning) watch.Stop();
+                if (lockTaken) Monitor.Exit(_parsingRunSyncObject);
                 if (_parsingSuspendLock.IsReadLockHeld)
                 {
                     _parsingSuspendLock.ExitReadLock();
                 }
-                if (lockTaken) Monitor.Exit(_parsingRunSyncObject);
             }
             if (watch != null) Logger.Debug("Parsing run finished after {0}s. (thread {1}).", watch.Elapsed.TotalSeconds, Thread.CurrentThread.ManagedThreadId);
         }

diff --git a/RubberduckTests/ParserState/ParserStateTests.cs b/RubberduckTests/ParserState/ParserStateTests.cs
@@ -162,6 +162,7 @@ public void Test_RPS_SuspendParser_Interrupted_Deadlock()
             // unwanted inlining of the tasks.
             // See: https://stackoverflow.com/questions/12245935/is-task-factory-startnew-guaranteed-to-use-another-thread-than-the-calling-thr
             var source = new CancellationTokenSource();
+            var token = source.Token;
             Task result2 = null;
 
             state.StateChanged += (o, e) =>
@@ -173,20 +174,20 @@ public void Test_RPS_SuspendParser_Interrupted_Deadlock()
                         wasSuspensionExecuted =
                             state.OnSuspendParser(this, () => { wasSuspended = state.Status == ParserState.Busy; },
                                 20);
-                    }, source.Token);
-                    result2.Wait(source.Token);
+                    }, token);
+                    result2.Wait(token);
                 }
             };
             var result1 = Task.Run(() =>
             {
                 state.OnParseRequested(this);
-            }, source.Token);
-            result1.Wait(source.Token);
+            }, token);
+            result1.Wait(token);
             while (result2 == null)
             {
                 Thread.Sleep(1);
             }
-            result2.Wait();
+            result2.Wait(token);
             Assert.IsFalse(wasSuspended, "wasSuspended was set to true");
             Assert.IsFalse(wasSuspensionExecuted, "wasSuspensionExecuted was set to true");
         }