Merge pull request #5273 from MDoerner/FixesAroundDeadlocks

Fixes around deadlocks

Author: retailcoder

Rubberduck.CodeAnalysis/Inspections/Results/IdentifierReferenceInspectionResult.cs

@@ -34,8 +34,8 @@ public IdentifierReferenceInspectionResult(IInspection inspection, string descri
 
         public override bool ChangesInvalidateResult(ICollection<QualifiedModuleName> modifiedModules)
         {
-            return modifiedModules.Contains(Target.QualifiedModuleName)
-                || base.ChangesInvalidateResult(modifiedModules);
+            return Target != null && modifiedModules.Contains(Target.QualifiedModuleName)
+                   || base.ChangesInvalidateResult(modifiedModules);
         }
     }
 }

Rubberduck.Core/UI/Command/RunSelectedTestMethodCommand.cs

@@ -50,7 +50,8 @@ private Declaration FindDeclarationFromSelection()
 
         private bool IsTestMethod(Declaration member)
         {
-            return member.DeclarationType == DeclarationType.Procedure
+            return member != null 
+                   && member.DeclarationType == DeclarationType.Procedure
                    && member.Annotations.Any(parseTreeAnnotation =>
                        parseTreeAnnotation.Annotation is TestMethodAnnotation);
         }

Rubberduck.UnitTesting/UnitTesting/TestEngine.cs

@@ -3,6 +3,7 @@
 using System.Diagnostics;
 using System.Linq;
 using System.Runtime.InteropServices;
+using System.Threading.Tasks;
 using NLog;
 using Rubberduck.Parsing.Annotations;
 using Rubberduck.Parsing.Symbols;
@@ -18,10 +19,8 @@ namespace Rubberduck.UnitTesting
     // FIXME litter logging around here
     internal class TestEngine : ITestEngine
     {
-        private static readonly ParserState[] AllowedRunStates = 
+        protected static readonly ParserState[] AllowedRunStates = 
         {
-            ParserState.ResolvedDeclarations,
-            ParserState.ResolvingReferences,
             ParserState.Ready
         };
 
@@ -172,13 +171,14 @@ public void RequestCancellation()
             CancellationRequested = true;
         }
 
-        private void RunInternal(IEnumerable<TestMethod> tests)
+        protected virtual void RunInternal(IEnumerable<TestMethod> tests)
         {
             if (!CanRun)
             {
                 return;
             }
-            _state.OnSuspendParser(this, AllowedRunStates, () => RunWhileSuspended(tests));
+            //We push the suspension to a background thread to avoid potential deadlocks if a parse is still running.
+            Task.Run(() => _state.OnSuspendParser(this, AllowedRunStates, () => RunWhileSuspended(tests)));
         }
 
         private void EnsureRubberduckIsReferencedForEarlyBoundTests()
@@ -199,7 +199,15 @@ private void EnsureRubberduckIsReferencedForEarlyBoundTests()
             }
         }
 
-        private void RunWhileSuspended(IEnumerable<TestMethod> tests)
+        protected void RunWhileSuspended(IEnumerable<TestMethod> tests)
+        {
+            //Running the tests has to be done on the UI thread, so we push the task to it from within suspension of the parser.
+            //We have to wait for the completion to make sure that the suspension only ends after tests have been completed.
+            var testTask = _uiDispatcher.StartTask(() => RunWhileSuspendedOnUiThread(tests));
+            testTask.Wait();
+        }
+
+        private void RunWhileSuspendedOnUiThread(IEnumerable<TestMethod> tests)
         {
             var testMethods = tests as IList<TestMethod> ?? tests.ToList();
             if (!testMethods.Any())

RubberduckTests/UnitTesting/MockedTestEngine.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
+using System.Threading.Tasks;
 using Moq;
 using NUnit.Framework;
 using Rubberduck.Parsing.UIContext;
@@ -47,8 +48,17 @@ internal class MockedTestEngine : IDisposable
 
         private MockedTestEngine()
         {
-            Dispatcher.Setup(d => d.InvokeAsync(It.IsAny<Action>())).Callback((Action action) => action.Invoke()).Verifiable();
-            
+            Dispatcher.Setup(d => d.InvokeAsync(It.IsAny<Action>()))
+                .Callback((Action action) => action.Invoke())
+                .Verifiable();
+            Dispatcher.Setup(d => d.StartTask(It.IsAny<Action>(), It.IsAny<TaskCreationOptions>()))
+                .Returns((Action action, TaskCreationOptions options) =>
+                    {
+                        action.Invoke();
+                        return Task.CompletedTask;
+                    })
+                .Verifiable();
+
             TypeLib.Setup(tlm => tlm.Dispose()).Verifiable();
             WrapperProvider.Setup(p => p.TypeLibWrapperFromProject(It.IsAny<string>())).Returns(TypeLib.Object).Verifiable();
 
@@ -64,7 +74,7 @@ public MockedTestEngine(string testModuleCode) : this()
 
             Vbe = builder.Build();
             ParserState = MockParser.Create(Vbe.Object).State;
-            TestEngine = new TestEngine(ParserState, _fakesFactory.Object, VbeInteraction.Object, WrapperProvider.Object, Dispatcher.Object, Vbe.Object);
+            TestEngine = new SynchronouslySuspendingTestEngine(ParserState, _fakesFactory.Object, VbeInteraction.Object, WrapperProvider.Object, Dispatcher.Object, Vbe.Object);
         }
 
         public MockedTestEngine(IReadOnlyList<string> moduleNames, IReadOnlyList<int> methodCounts) : this()
@@ -87,7 +97,7 @@ public MockedTestEngine(IReadOnlyList<string> moduleNames, IReadOnlyList<int> me
             project.AddProjectToVbeBuilder();
             Vbe = builder.Build();
             ParserState = MockParser.Create(Vbe.Object).State;
-            TestEngine = new TestEngine(ParserState, _fakesFactory.Object, VbeInteraction.Object, WrapperProvider.Object, Dispatcher.Object, Vbe.Object);
+            TestEngine = new SynchronouslySuspendingTestEngine(ParserState, _fakesFactory.Object, VbeInteraction.Object, WrapperProvider.Object, Dispatcher.Object, Vbe.Object);
         }
 
         public MockedTestEngine(int testMethodCount) 
@@ -246,5 +256,32 @@ Public Sub TestCleanup()
     'this method runs after every test in the module.
 End Sub
 ";
+
+        private class SynchronouslySuspendingTestEngine : TestEngine
+        {
+            private readonly RubberduckParserState _state;
+
+            public SynchronouslySuspendingTestEngine(
+                RubberduckParserState state,
+                IFakesFactory fakesFactory,
+                IVBEInteraction declarationRunner,
+                ITypeLibWrapperProvider wrapperProvider,
+                IUiDispatcher uiDispatcher,
+                IVBE vbe)
+                : base(state, fakesFactory, declarationRunner, wrapperProvider, uiDispatcher, vbe)
+            {
+                _state = state;
+            }
+
+            protected override void RunInternal(IEnumerable<TestMethod> tests)
+            {
+                if (!CanRun)
+                {
+                    return;
+                }
+                //We have to do this on the same thread here to guarantee that the actions runs before the assert in the unit tests is called.
+                _state.OnSuspendParser(this, AllowedRunStates, () => RunWhileSuspended(tests));
+            }
+        }
     }
 }