Skip to content

Commit

Permalink
Merge pull request #66886 from dibarbet/workspace_diagnostics_poll
Browse files Browse the repository at this point in the history 
Fix workspace pull diagnostics
  • Loading branch information
@dibarbet
dibarbet authored Mar 2, 2023
2 parents 66a3ae6 + 55bf9d3 commit 81404ba
Show file tree
Hide file tree
Showing 6 changed files with 229 additions and 53 deletions.
14 changes: 14 additions & 0 deletions src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ protected abstract ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagno

protected abstract string? GetDiagnosticCategory(TDiagnosticsParams diagnosticsParams);

/// <summary>
/// Used by public workspace pull diagnostics to allow it to keep the connection open until
/// changes occur to avoid the client spamming the server with requests.
/// </summary>
protected virtual Task WaitForChangesAsync(RequestContext context, CancellationToken cancellationToken)
{
return Task.CompletedTask;
}

public async Task<TReturn?> HandleRequestAsync(
TDiagnosticsParams diagnosticsParams, RequestContext context, CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -182,6 +191,11 @@ await ComputeAndReportCurrentDiagnosticsAsync(
}
}

// Some implementations of the spec will re-open requests as soon as we close them, spamming the server.
// In those cases, we wait for the implementation to indicate that changes have occurred, then we close the connection
// so that the client asks us again.
await WaitForChangesAsync(context, cancellationToken).ConfigureAwait(false);

// If we had a progress object, then we will have been reporting to that. Otherwise, take what we've been
// collecting and return that.
context.TraceInformation($"{this.GetType()} finished getting diagnostics");
Expand Down
9 changes: 7 additions & 2 deletions ...Server/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,34 @@
using Microsoft.CodeAnalysis.EditAndContinue;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;

[ExportCSharpVisualBasicLspServiceFactory(typeof(PublicWorkspacePullDiagnosticsHandler)), Shared]
internal class PublicWorkspacePullDiagnosticHandlerFactory : ILspServiceFactory
{
private readonly LspWorkspaceRegistrationService _registrationService;
private readonly IDiagnosticAnalyzerService _analyzerService;
private readonly EditAndContinueDiagnosticUpdateSource _editAndContinueDiagnosticUpdateSource;
private readonly IGlobalOptionService _globalOptions;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public PublicWorkspacePullDiagnosticHandlerFactory(
LspWorkspaceRegistrationService registrationService,
IDiagnosticAnalyzerService analyzerService,
EditAndContinueDiagnosticUpdateSource editAndContinueDiagnosticUpdateSource,
IGlobalOptionService globalOptions)
{
_registrationService = registrationService;
_analyzerService = analyzerService;
_editAndContinueDiagnosticUpdateSource = editAndContinueDiagnosticUpdateSource;
_globalOptions = globalOptions;
}

public ILspService CreateILspService(LspServices lspServices, WellKnownLspServerKinds serverKind)
=> new PublicWorkspacePullDiagnosticsHandler(_analyzerService, _editAndContinueDiagnosticUpdateSource, _globalOptions);
{
var workspaceManager = lspServices.GetRequiredService<LspWorkspaceManager>();
return new PublicWorkspacePullDiagnosticsHandler(workspaceManager, _registrationService, _analyzerService, _editAndContinueDiagnosticUpdateSource, _globalOptions);
}
}
72 changes: 71 additions & 1 deletion ...nguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.CodeAnalysis.EditAndContinue;
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;

Expand All @@ -21,14 +22,38 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;
using WorkspaceDiagnosticPartialReport = SumType<WorkspaceDiagnosticReport, WorkspaceDiagnosticReportPartialResult>;

[Method(Methods.WorkspaceDiagnosticName)]
internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler<WorkspaceDiagnosticParams, WorkspaceDiagnosticPartialReport, WorkspaceDiagnosticReport?>
internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler<WorkspaceDiagnosticParams, WorkspaceDiagnosticPartialReport, WorkspaceDiagnosticReport?>, IDisposable
{
private readonly LspWorkspaceRegistrationService _workspaceRegistrationService;
private readonly LspWorkspaceManager _workspaceManager;

/// <summary>
/// Flag that represents whether the LSP view of the world has changed.
/// It is totally fine for this to somewhat over-report changes
/// as it is an optimization used to delay closing workspace diagnostic requests
/// until something has changed.
/// </summary>
private int _lspChanged = 0;

public PublicWorkspacePullDiagnosticsHandler(
LspWorkspaceManager workspaceManager,
LspWorkspaceRegistrationService registrationService,
IDiagnosticAnalyzerService analyzerService,
EditAndContinueDiagnosticUpdateSource editAndContinueDiagnosticUpdateSource,
IGlobalOptionService globalOptions)
: base(analyzerService, editAndContinueDiagnosticUpdateSource, globalOptions)
{
_workspaceRegistrationService = registrationService;
_workspaceRegistrationService.LspSolutionChanged += OnLspSolutionChanged;

_workspaceManager = workspaceManager;
_workspaceManager.LspTextChanged += OnLspTextChanged;
}

public void Dispose()
{
_workspaceManager.LspTextChanged -= OnLspTextChanged;
_workspaceRegistrationService.LspSolutionChanged -= OnLspSolutionChanged;
}

/// <summary>
Expand Down Expand Up @@ -118,4 +143,49 @@ protected override ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagno
}
}).ToImmutableArray();
}

private void OnLspSolutionChanged(object? sender, WorkspaceChangeEventArgs e)
{
UpdateLspChanged();
}

private void OnLspTextChanged(object? sender, EventArgs e)
{
UpdateLspChanged();
}

private void UpdateLspChanged()
{
Interlocked.Exchange(ref _lspChanged, 1);
}

protected override async Task WaitForChangesAsync(RequestContext context, CancellationToken cancellationToken)
{
// Spin waiting until our LSP change flag has been set. When the flag is set (meaning LSP has changed),
// we reset the flag to false and exit out of the loop allowing the request to close.
// The client will automatically trigger a new request as soon as we close it, bringing us up to date on diagnostics.
while (Interlocked.CompareExchange(ref _lspChanged, value: 0, comparand: 1) == 0)
{
// There have been no changes between now and when the last request finished - we will hold the connection open while we poll for changes.
await Task.Delay(TimeSpan.FromMilliseconds(100), cancellationToken).ConfigureAwait(false);
}

context.TraceInformation("Closing workspace/diagnostics request");
// We've hit a change, so we close the current request to allow the client to open a new one.
return;
}

internal TestAccessor GetTestAccessor() => new(this);

internal readonly struct TestAccessor
{
private readonly PublicWorkspacePullDiagnosticsHandler _handler;

public TestAccessor(PublicWorkspacePullDiagnosticsHandler handler)
{
_handler = handler;
}

public void TriggerConnectionClose() => _handler.UpdateLspChanged();
}
}
8 changes: 8 additions & 0 deletions src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ public LspWorkspaceManager(
_lspWorkspaceRegistrationService = lspWorkspaceRegistrationService;
}

public EventHandler<EventArgs>? LspTextChanged;

#region Implementation of IDocumentChangeTracker

/// <summary>
Expand All @@ -102,6 +104,8 @@ public void StartTracking(Uri uri, SourceText documentText)

// If LSP changed, we need to compare against the workspace again to get the updated solution.
_cachedLspSolutions.Clear();

LspTextChanged?.Invoke(this, EventArgs.Empty);
}

/// <summary>
Expand All @@ -120,6 +124,8 @@ public void StopTracking(Uri uri)

// Also remove it from our loose files workspace if it is still there.
_lspMiscellaneousFilesWorkspace.TryRemoveMiscellaneousDocument(uri);

LspTextChanged?.Invoke(this, EventArgs.Empty);
}

/// <summary>
Expand All @@ -135,6 +141,8 @@ public void UpdateTrackedDocument(Uri uri, SourceText newSourceText)

// If LSP changed, we need to compare against the workspace again to get the updated solution.
_cachedLspSolutions.Clear();

LspTextChanged?.Invoke(this, EventArgs.Empty);
}

public ImmutableDictionary<Uri, SourceText> GetTrackedLspText() => _trackedDocuments;
Expand Down
131 changes: 81 additions & 50 deletions src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.CSharp.RemoveUnusedParametersAndValues;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.LanguageServer.Handler;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Microsoft.CodeAnalysis.TaskList;
Expand Down Expand Up @@ -61,73 +62,103 @@ private protected static async Task<ImmutableArray<TestDiagnosticResult>> RunGet
{
var optionService = testLspServer.TestWorkspace.GetService<IGlobalOptionService>();
optionService.SetGlobalOption(TaskListOptionsStorage.ComputeTaskListItemsForClosedFiles, includeTaskListItems);
await testLspServer.WaitForDiagnosticsAsync();

if (useVSDiagnostics)
{
BufferedProgress<VSInternalWorkspaceDiagnosticReport[]>? progress = useProgress ? BufferedProgress.Create<VSInternalWorkspaceDiagnosticReport[]>(null) : null;
var diagnostics = await testLspServer.ExecuteRequestAsync<VSInternalWorkspaceDiagnosticsParams, VSInternalWorkspaceDiagnosticReport[]>(
VSInternalMethods.WorkspacePullDiagnosticName,
CreateWorkspaceDiagnosticParams(previousResults, progress, category),
CancellationToken.None).ConfigureAwait(false);
return await RunVSGetWorkspacePullDiagnosticsAsync(testLspServer, previousResults, useProgress, category);
}
else
{
return await RunPublicGetWorkspacePullDiagnosticsAsync(testLspServer, previousResults, useProgress, triggerConnectionClose: true);
}
}

if (useProgress)
{
Assert.Null(diagnostics);
diagnostics = progress!.Value.GetFlattenedValues();
}
private protected static async Task<ImmutableArray<TestDiagnosticResult>> RunVSGetWorkspacePullDiagnosticsAsync(
TestLspServer testLspServer,
ImmutableArray<(string resultId, TextDocumentIdentifier identifier)>? previousResults = null,
bool useProgress = false,
string? category = null)
{
await testLspServer.WaitForDiagnosticsAsync();

AssertEx.NotNull(diagnostics);
return diagnostics.Select(d => new TestDiagnosticResult(d.TextDocument!, d.ResultId!, d.Diagnostics)).ToImmutableArray();
BufferedProgress<VSInternalWorkspaceDiagnosticReport[]>? progress = useProgress ? BufferedProgress.Create<VSInternalWorkspaceDiagnosticReport[]>(null) : null;
var diagnostics = await testLspServer.ExecuteRequestAsync<VSInternalWorkspaceDiagnosticsParams, VSInternalWorkspaceDiagnosticReport[]>(
VSInternalMethods.WorkspacePullDiagnosticName,
CreateWorkspaceDiagnosticParams(previousResults, progress, category),
CancellationToken.None).ConfigureAwait(false);

if (useProgress)
{
Assert.Null(diagnostics);
diagnostics = progress!.Value.GetFlattenedValues();
}
else

AssertEx.NotNull(diagnostics);
return diagnostics.Select(d => new TestDiagnosticResult(d.TextDocument!, d.ResultId!, d.Diagnostics)).ToImmutableArray();
}

private protected static async Task<ImmutableArray<TestDiagnosticResult>> RunPublicGetWorkspacePullDiagnosticsAsync(
TestLspServer testLspServer,
ImmutableArray<(string resultId, TextDocumentIdentifier identifier)>? previousResults = null,
bool useProgress = false,
bool triggerConnectionClose = true)
{
await testLspServer.WaitForDiagnosticsAsync();

BufferedProgress<WorkspaceDiagnosticPartialReport>? progress = useProgress ? BufferedProgress.Create<WorkspaceDiagnosticPartialReport>(null) : null;
var diagnosticsTask = testLspServer.ExecuteRequestAsync<WorkspaceDiagnosticParams, WorkspaceDiagnosticReport?>(
Methods.WorkspaceDiagnosticName,
CreateProposedWorkspaceDiagnosticParams(previousResults, progress),
CancellationToken.None).ConfigureAwait(false);

if (triggerConnectionClose)
{
BufferedProgress<WorkspaceDiagnosticPartialReport>? progress = useProgress ? BufferedProgress.Create<WorkspaceDiagnosticPartialReport>(null) : null;
var returnedResult = await testLspServer.ExecuteRequestAsync<WorkspaceDiagnosticParams, WorkspaceDiagnosticReport?>(
Methods.WorkspaceDiagnosticName,
CreateProposedWorkspaceDiagnosticParams(previousResults, progress),
CancellationToken.None).ConfigureAwait(false);
// Public spec diagnostics wait for a change before closing the connection so we manually tell it to close here to let the test finish.
var service = testLspServer.GetRequiredLspService<PublicWorkspacePullDiagnosticsHandler>();
service.GetTestAccessor().TriggerConnectionClose();
}

if (useProgress)
{
Assert.Empty(returnedResult!.Items);
var progressValues = progress!.Value.GetValues();
Assert.NotNull(progressValues);
return progressValues.SelectMany(value => value.Match(v => v.Items, v => v.Items)).Select(diagnostics => ConvertWorkspaceDiagnosticResult(diagnostics)).ToImmutableArray();
var returnedResult = await diagnosticsTask;

}
if (useProgress)
{
Assert.Empty(returnedResult!.Items);
var progressValues = progress!.Value.GetValues();
Assert.NotNull(progressValues);
return progressValues.SelectMany(value => value.Match(v => v.Items, v => v.Items)).Select(diagnostics => ConvertWorkspaceDiagnosticResult(diagnostics)).ToImmutableArray();

AssertEx.NotNull(returnedResult);
return returnedResult.Items.Select(diagnostics => ConvertWorkspaceDiagnosticResult(diagnostics)).ToImmutableArray();
}

static WorkspaceDiagnosticParams CreateProposedWorkspaceDiagnosticParams(
AssertEx.NotNull(returnedResult);
return returnedResult.Items.Select(diagnostics => ConvertWorkspaceDiagnosticResult(diagnostics)).ToImmutableArray();
}

private static WorkspaceDiagnosticParams CreateProposedWorkspaceDiagnosticParams(
ImmutableArray<(string resultId, TextDocumentIdentifier identifier)>? previousResults = null,
IProgress<WorkspaceDiagnosticPartialReport>? progress = null)
{
var previousResultsLsp = previousResults?.Select(r => new PreviousResultId
{
var previousResultsLsp = previousResults?.Select(r => new PreviousResultId
{
Uri = r.identifier.Uri,
Value = r.resultId
}).ToArray() ?? Array.Empty<PreviousResultId>();
return new WorkspaceDiagnosticParams
{
PreviousResultId = previousResultsLsp,
PartialResultToken = progress
};
}
Uri = r.identifier.Uri,
Value = r.resultId
}).ToArray() ?? Array.Empty<PreviousResultId>();
return new WorkspaceDiagnosticParams
{
PreviousResultId = previousResultsLsp,
PartialResultToken = progress
};
}

static TestDiagnosticResult ConvertWorkspaceDiagnosticResult(SumType<WorkspaceFullDocumentDiagnosticReport, WorkspaceUnchangedDocumentDiagnosticReport> workspaceReport)
private static TestDiagnosticResult ConvertWorkspaceDiagnosticResult(SumType<WorkspaceFullDocumentDiagnosticReport, WorkspaceUnchangedDocumentDiagnosticReport> workspaceReport)
{
if (workspaceReport.Value is WorkspaceFullDocumentDiagnosticReport fullReport)
{
if (workspaceReport.Value is WorkspaceFullDocumentDiagnosticReport fullReport)
{
return new TestDiagnosticResult(new TextDocumentIdentifier { Uri = fullReport.Uri }, fullReport.ResultId!, fullReport.Items);
}
else
{
var unchangedReport = (WorkspaceUnchangedDocumentDiagnosticReport)workspaceReport.Value!;
return new TestDiagnosticResult(new TextDocumentIdentifier { Uri = unchangedReport.Uri }, unchangedReport.ResultId!, null);
}
return new TestDiagnosticResult(new TextDocumentIdentifier { Uri = fullReport.Uri }, fullReport.ResultId!, fullReport.Items);
}
else
{
var unchangedReport = (WorkspaceUnchangedDocumentDiagnosticReport)workspaceReport.Value!;
return new TestDiagnosticResult(new TextDocumentIdentifier { Uri = unchangedReport.Uri }, unchangedReport.ResultId!, null);
}
}

Expand Down
Loading

0 comments on commit 81404ba

Please sign in to comment.