Remove the unnecessary deadlock check from ParseInternal method.

Improve the thread management in the unit tests
rubberduck-vba · Jun 17, 2018 · 10f4044 · 10f4044
1 parent 98dd544
commit 10f4044
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 73 deletions.
diff --git a/Rubberduck.Parsing/VBA/ParseCoordinator.cs b/Rubberduck.Parsing/VBA/ParseCoordinator.cs
@@ -84,6 +84,10 @@ private void ReparseRequested(object sender, EventArgs e)
             {
                 try
                 {
+                    // Removes the need for recursion policy. In unlikely event where the 
+                    // same thread that is executing the busyAction of the suspendRequest
+                    // has requested a reparse, we can skip taking a read lock, since we already
+                    // have the write lock further up the thread's call stack.
                     if (!_parsingSuspendLock.IsWriteLockHeld)
                     {
                         _parsingSuspendLock.EnterReadLock();
@@ -205,7 +209,6 @@ private void Cancel(bool createNewTokenSource = true)
         /// <summary>
         /// For the use of tests only
         /// </summary>
-        /// 
         public void Parse(CancellationTokenSource token)
         {
             SetSavedCancellationTokenSource(token);
@@ -215,7 +218,6 @@ public void Parse(CancellationTokenSource token)
         /// <summary>
         /// For the use of tests only
         /// </summary>
-        /// 
         private void SetSavedCancellationTokenSource(CancellationTokenSource tokenSource)
         {
             var oldTokenSource = _currentCancellationTokenSource;
@@ -230,7 +232,6 @@ private void ParseInternal(CancellationToken token)
             var lockTaken = false;
             try
             {
-                _parsingSuspendLock.EnterReadLock();
                 Monitor.Enter(_parsingRunSyncObject, ref lockTaken);
                 ParseAllInternal(this, token);
             }
@@ -244,10 +245,6 @@ private void ParseInternal(CancellationToken token)
                 {
                     Monitor.Exit(_parsingRunSyncObject);
                 }
-                if (_parsingSuspendLock.IsReadLockHeld)
-                {
-                    _parsingSuspendLock.ExitReadLock();
-                }
             }
         }
 

diff --git a/RubberduckTests/ParserState/ParserStateTests.cs b/RubberduckTests/ParserState/ParserStateTests.cs
@@ -119,14 +119,18 @@ public void Test_RPS_SuspendParser_Interrupted_IsQueued()
             {
                 if (e.State == ParserState.Started)
                 {
-                    result = Task.Run(() =>
+                    if (result == null)
                     {
-                        state.OnSuspendParser(this, () =>
+                        result = Task.Run(() =>
                         {
-                            wasBusy = state.Status == ParserState.Busy;
+                            state.OnSuspendParser(this, () =>
+                            {
+                                wasBusy = state.Status == ParserState.Busy;
+                            });
                         });
-                    });
-                    return;
+                        result.Wait();
+                        wasBusy = false;
+                    }
                 }
 
                 if (e.State == ParserState.Ready && wasBusy)
@@ -164,12 +168,16 @@ public void Test_RPS_SuspendParser_Interrupted_TwoRequests_IsQueued()
             {
                 if (e.State == ParserState.Started && !wasRunning)
                 {
-                    result1 = Task.Run(() =>
+                    if (result1 == null)
                     {
-                        wasRunning = true;
-                        result2 = Task.Run(() => state.OnParseRequested(this));
-                    });
-                    return;
+                        result1 = Task.Run(() =>
+                        {
+                            wasRunning = true;
+                            result2 = Task.Run(() => state.OnParseRequested(this));
+                        });
+                        result1.Wait();
+                        return;
+                    }
                 }
 
                 if (e.State == ParserState.Started && wasRunning)
@@ -208,61 +216,5 @@ public void Test_RPS_SuspendParser_Interrupted_TwoRequests_IsQueued()
             suspendTask.Wait();
             Assert.AreEqual(1, reparseAfterBusy);
         }
-
-        [Test]
-        [Category("ParserState")]
-        public void Test_RPS_SuspendParser_Interrupted_TwoRequests_Sync_IsQueued()
-        {
-            var vbe = MockVbeBuilder.BuildFromSingleModule("", ComponentType.StandardModule, out var _);
-            var state = MockParser.CreateAndParse(vbe.Object);
-
-            var wasRunning = false;
-            var wasBusy = false;
-            var reparseAfterBusy = 0;
-            Task result1 = null;
-            Task result2 = null;
-
-            state.StateChanged += (o, e) =>
-            {
-                if (e.State == ParserState.Started && !wasRunning)
-                {
-                    result1 = Task.Run(() =>
-                    {
-                        wasRunning = true;
-                        result2 = Task.Run(() => state.OnParseRequested(this));
-                    });
-                    return;
-                }
-
-                if (e.State == ParserState.Started && wasRunning)
-                {
-                    state.OnSuspendParser(this, () =>
-                    {
-                        wasBusy = state.Status == ParserState.Busy;
-                    });
-                    return;
-                }
-
-                if (e.State == ParserState.Ready && wasBusy)
-                {
-                    reparseAfterBusy++;
-                }
-            };
-
-            state.OnParseRequested(this);
-            while (result1 == null)
-            {
-                Thread.Sleep(1);
-            }
-            result1.Wait();
-            while (result2 == null)
-            {
-                Thread.Sleep(1);
-            }
-            result2.Wait();
-
-            Assert.IsFalse(wasBusy);
-            Assert.AreEqual(0, reparseAfterBusy);
-        }
     }
 }