Skip to content

Commit

Permalink
Fix crash caused by daemon.Start race condition (#1000)
Browse files Browse the repository at this point in the history 
* Fix crash caused by daemon.Start race condition
Fixed #999
  • Loading branch information
duncanp-sonar authored Aug 7, 2019
1 parent 38aa1be commit 47c45d6
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 39 deletions.
149 changes: 120 additions & 29 deletions src/Integration.Vsix.UnitTests/SonarLintDaemon/DaemonAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,7 @@ public void RequestAnalysis_Started_AnalysisRequested()
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);
CheckTelemetryManagerCallCount("js", 1);

// 2. Check the event handlers have been unsubscribed
dummyDaemon.SimulateDaemonReady(null);
dummyInstaller.SimulateInstallFinished(null);

// Assert - not other calls
dummyInstaller.InstallCallCount.Should().Be(0);
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);
CheckEventHandlersUnsubscribed();
CheckTelemetryManagerTotalCallCount(1);
}

Expand Down Expand Up @@ -153,6 +146,42 @@ public void RequestAnalysis_NotStarted_StartThenRequestAnalysis()
CheckTelemetryManagerTotalCallCount(1);
}

[TestMethod]
public void RequestAnalysis_NotStarted_StartThenRequestAnalysis_NonCriticalExceptionInMakeRequestIsSuppressed()
{
// Arrange
dummyDaemon.SupportedLanguages = new[] { SonarLanguage.Javascript };
dummyInstaller.IsInstalledReturnValue = true;
dummyDaemon.IsRunning = false;

bool requestAnalysisOpInvoked = false;
dummyDaemon.RequestAnalysisOperation = () =>
{
requestAnalysisOpInvoked = true;
throw new InvalidOperationException("xxx");
};

// 1. Make the request
analyzer.RequestAnalysis("path", "charset", new[] { SonarLanguage.Javascript }, null, null);

dummyDaemon.StartCallCount.Should().Be(1);
dummyDaemon.RequestAnalysisCallCount.Should().Be(0); // should be waiting for the daemon to be ready

// 2. Simulate daemon being ready
dummyDaemon.SimulateDaemonReady(null);
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);
CheckTelemetryManagerCallCount("js", 1);

requestAnalysisOpInvoked.Should().BeTrue();

// Sanity check of all of the call counts
dummyInstaller.InstallCallCount.Should().Be(0);
dummyDaemon.StartCallCount.Should().Be(1);
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);

CheckEventHandlersUnsubscribed();
}

[TestMethod]
public void RequestAnalysis_NotInstalled_InstallThenStartThenRequestAnalysis()
{
Expand All @@ -178,15 +207,71 @@ public void RequestAnalysis_NotInstalled_InstallThenStartThenRequestAnalysis()
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);
CheckTelemetryManagerCallCount("js", 1);

// 4. Check the event handlers have been unsubscribed
dummyDaemon.SimulateDaemonReady(null);
dummyInstaller.SimulateInstallFinished(null);
CheckEventHandlersUnsubscribed();
}

[TestMethod]
public void RequestAnalysis_NotInstalled_InstallCalled_DaemonAlreadyRunningSoStartNotCalled_RequestAnalysisCalled()
{
// Related to https://github.com/SonarSource/sonarlint-visualstudio/issues/999

// Arrange
dummyDaemon.SupportedLanguages = new[] { SonarLanguage.Javascript };
dummyInstaller.IsInstalledReturnValue = false;
dummyDaemon.IsRunning = false;

// 1. Make the request
analyzer.RequestAnalysis("path", "charset", new[] { SonarLanguage.Javascript }, null, null);

// Sanity check of all of the call counts
dummyInstaller.InstallCallCount.Should().Be(1);
dummyDaemon.StartCallCount.Should().Be(1);
dummyDaemon.StartCallCount.Should().Be(0); // should be waiting for the daemon to be installed
dummyDaemon.RequestAnalysisCallCount.Should().Be(0);

// 2. Simulate daemon started by another caller, then send the "installation complete" notification
dummyDaemon.IsRunning = true;
dummyInstaller.SimulateInstallFinished(new AsyncCompletedEventArgs(null, false /* cancelled */, null));

// Should not call Start in this case: should just request the analysis
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);
CheckTelemetryManagerTotalCallCount(1);

CheckEventHandlersUnsubscribed();
}

[TestMethod]
public void RequestAnalysis_NotInstalled_InstallCalled_DaemonAlreadyRunning_RequestAnalysisCalled_NonCriticalIsSuppressed()
{
// Related to https://github.com/SonarSource/sonarlint-visualstudio/issues/999

// Arrange
dummyDaemon.SupportedLanguages = new[] { SonarLanguage.Javascript };
dummyInstaller.IsInstalledReturnValue = false;
dummyDaemon.IsRunning = false;

bool requestAnalysisOpInvoked = false;
dummyDaemon.RequestAnalysisOperation = () =>
{
requestAnalysisOpInvoked = true;
throw new InvalidOperationException("xxx");
};

// 1. Make the request
analyzer.RequestAnalysis("path", "charset", new[] { SonarLanguage.Javascript }, null, null);

dummyInstaller.InstallCallCount.Should().Be(1);
dummyDaemon.StartCallCount.Should().Be(0); // should be waiting for the daemon to be installed
dummyDaemon.RequestAnalysisCallCount.Should().Be(0);

// 2. Simulate daemon started by another caller, then send the "installation complete" notification
dummyDaemon.IsRunning = true;
dummyInstaller.SimulateInstallFinished(new AsyncCompletedEventArgs(null, false /* cancelled */, null));

// Exception should be suppressed
requestAnalysisOpInvoked.Should().BeTrue();
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(1);

CheckEventHandlersUnsubscribed();
}

[TestMethod]
Expand All @@ -212,15 +297,7 @@ public void RequestAnalysis_NotInstalled_ErrorOnInstall_StartNotCalled()
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(0); // should be waiting for the daemon to be ready

// 3. Check the event handlers have been unsubscribed
dummyDaemon.SimulateDaemonReady(null);
dummyInstaller.SimulateInstallFinished(null);

// Sanity check of all of the call counts
dummyInstaller.InstallCallCount.Should().Be(1);
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(0);
CheckTelemetryManagerTotalCallCount(0);
CheckEventHandlersUnsubscribed();
}

[TestMethod]
Expand All @@ -246,15 +323,29 @@ public void RequestAnalysis_NotInstalled_InstallCancelled_StartNotCalled()
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(0); // should be waiting for the daemon to be ready

// 3. Check the event handlers have been unsubscribed
CheckEventHandlersUnsubscribed();
}

private void CheckEventHandlersUnsubscribed()
{
// Check that firing the events doesn't result in any more method calls.

// Store the current counts
var installCallCount = dummyInstaller.InstallCallCount;
var startCallCount = dummyDaemon.StartCallCount;
var requestCallCount = dummyDaemon.RequestAnalysisCallCount;

// Make the telemetry manager throw if it is called again
telemetryManagerMock.Setup(x => x.LanguageAnalyzed(It.IsAny<string>())).Throws<InvalidOperationException>();

// Fire the events
dummyDaemon.SimulateDaemonReady(null);
dummyInstaller.SimulateInstallFinished(null);

// Sanity check of all of the call counts
dummyInstaller.InstallCallCount.Should().Be(1);
dummyDaemon.StartCallCount.Should().Be(0);
dummyDaemon.RequestAnalysisCallCount.Should().Be(0);
CheckTelemetryManagerTotalCallCount(0);
// Check the call counts haven't changed
dummyInstaller.InstallCallCount.Should().Be(installCallCount);
dummyDaemon.StartCallCount.Should().Be(startCallCount);
dummyDaemon.RequestAnalysisCallCount.Should().Be(requestCallCount);
}

private void CheckTelemetryManagerCallCount(string languageKey, int expectedCallCount)
Expand Down
12 changes: 12 additions & 0 deletions src/Integration.Vsix.UnitTests/SonarLintDaemon/Helpers/DummySonarLintDaemon.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ public class DummySonarLintDaemon : ISonarLintDaemon
public int StartCallCount { get; private set; }
public int RequestAnalysisCallCount { get; private set; }

/// <summary>
/// Optional - code to execute when Start is called
/// </summary>
public Action StartOperation { get; set; }

/// <summary>
/// Optional - code execute when RequestAnalysis is called
/// </summary>
public Action RequestAnalysisOperation { get; set; }

public void SimulateDaemonReady(EventArgs args)
{
this.IsRunning = true;
Expand Down Expand Up @@ -62,11 +72,13 @@ public bool IsAnalysisSupported(IEnumerable<SonarLanguage> languages)
public void RequestAnalysis(string path, string charset, IEnumerable<SonarLanguage> detectedLanguages, IIssueConsumer consumer, ProjectItem projectItem)
{
RequestAnalysisCallCount++;
RequestAnalysisOperation?.Invoke();
}

public void Start()
{
StartCallCount++;
StartOperation?.Invoke();
}

public void Stop()
Expand Down
7 changes: 4 additions & 3 deletions src/Integration.Vsix.UnitTests/SonarLintDaemon/SonarLintDaemonTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
using System;
using System.ComponentModel;
using System.IO;
using System.Net;
using System.Threading;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
Expand Down Expand Up @@ -125,9 +124,11 @@ public void IsInstalled_ActivateAdditionalLanguagesIsFalse_Start_NotStarted()
public void Run_Without_Install()
{
dummyInstaller.IsInstalledReturnValue = false;
Action op = () => testableDaemon.Start();
testableDaemon.Start();

op.Should().ThrowExactly<InvalidOperationException>().And.Message.Should().Be("Daemon is not installed");
// Should not throw as this could crash VS: https://github.com/SonarSource/sonarlint-visualstudio/issues/999
// ...but daemon should not start either.
testableDaemon.IsRunning.Should().BeFalse();
}

[TestMethod]
Expand Down
42 changes: 37 additions & 5 deletions src/Integration.Vsix/SonarLintDaemon/DaemonAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
using System.Diagnostics;
using System.Linq;
using EnvDTE;
using Microsoft.VisualStudio;
using Microsoft.VisualStudio.Shell;

namespace SonarLint.VisualStudio.Integration.Vsix
{
Expand Down Expand Up @@ -112,6 +114,9 @@ public DelayedRequest(DaemonAnalyzer daemonAnalyzer, string path, string charset

public void Execute()
{
// Note: called on the UI thread so an unhandled exception will crash VS.
// However, the only excecution path to this point should be from our tagger which
// should handle any exceptions.
if (!daemonInstaller.IsInstalled())
{
daemonInstaller.InstallationCompleted += HandleInstallCompleted;
Expand Down Expand Up @@ -140,18 +145,45 @@ private void MakeRequest()

private void HandleInstallCompleted(object sender, AsyncCompletedEventArgs e)
{
daemonInstaller.InstallationCompleted -= HandleInstallCompleted;
// Could be on the UI thread -> unhandled exceptions will crash VS
// e.g. https://github.com/SonarSource/sonarlint-visualstudio/issues/999
try
{
daemonInstaller.InstallationCompleted -= HandleInstallCompleted;

if (e.Error == null && !e.Cancelled)
if (e.Error == null && !e.Cancelled)
{
// Potential race condition: the daemon might already have been started
if (daemon.IsRunning)
{
MakeRequest();
}
else
{
daemon.Ready += HandleDaemonReady;
daemon.Start();
}
}
}
catch(Exception ex) when (!ErrorHandler.IsCriticalException(ex))
{
daemon.Ready += HandleDaemonReady;
daemon.Start();
// Squash non-critical exceptions
Debug.WriteLine($"Error handling daemon installation complete notification: {ex.ToString()}");
}
}

private void HandleDaemonReady(object sender, EventArgs e)
{
MakeRequest();
// Could be on the UI thread -> unhandled exceptions will crash VS
try
{
MakeRequest();
}
catch (Exception ex) when (!ErrorHandler.IsCriticalException(ex))
{
// Squash non-critical exceptions
Debug.WriteLine($"Error handling daemon ready notification: {ex.ToString()}");
}
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/Integration.Vsix/SonarLintDaemon/SonarLintDaemon.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ public void Start()
{
if (IsRunning)
{
throw new InvalidOperationException("Process already running");
logger.WriteLine("Daemon is already running");
return;
}

if (!installer.IsInstalled())
{
throw new InvalidOperationException("Daemon is not installed");
logger.WriteLine("Daemon is not installed");
return;
}

if (!settings.IsActivateMoreEnabled)
Expand Down

0 comments on commit 47c45d6

Please sign in to comment.