diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index 957abcc2eeb84..9feca8653fab0 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -113,6 +113,15 @@ protected abstract ValueTask> GetOrderedDiagno protected abstract string? GetDiagnosticCategory(TDiagnosticsParams diagnosticsParams); + /// + /// 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. + /// + protected virtual Task WaitForChangesAsync(RequestContext context, CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + public async Task HandleRequestAsync( TDiagnosticsParams diagnosticsParams, RequestContext context, CancellationToken cancellationToken) { @@ -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"); diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs index be7cf451b1b2b..dfca2c14ca1ac 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs @@ -8,13 +8,13 @@ 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; @@ -22,15 +22,20 @@ internal class PublicWorkspacePullDiagnosticHandlerFactory : ILspServiceFactory [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(); + return new PublicWorkspacePullDiagnosticsHandler(workspaceManager, _registrationService, _analyzerService, _editAndContinueDiagnosticUpdateSource, _globalOptions); + } } diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs index 2c7d0c57c624d..9855681054b0a 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs @@ -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; @@ -21,14 +22,38 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public; using WorkspaceDiagnosticPartialReport = SumType; [Method(Methods.WorkspaceDiagnosticName)] -internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler +internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler, IDisposable { + private readonly LspWorkspaceRegistrationService _workspaceRegistrationService; + private readonly LspWorkspaceManager _workspaceManager; + + /// + /// 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. + /// + 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; } /// @@ -118,4 +143,49 @@ protected override ValueTask> 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(); + } } diff --git a/src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs b/src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs index dd5277612f4ba..54c06e2d898fa 100644 --- a/src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs +++ b/src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs @@ -87,6 +87,8 @@ public LspWorkspaceManager( _lspWorkspaceRegistrationService = lspWorkspaceRegistrationService; } + public EventHandler? LspTextChanged; + #region Implementation of IDocumentChangeTracker /// @@ -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); } /// @@ -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); } /// @@ -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 GetTrackedLspText() => _trackedDocuments; diff --git a/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs b/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs index 2623bbd39c0be..95a5428e84593 100644 --- a/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs +++ b/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs @@ -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; @@ -61,73 +62,103 @@ private protected static async Task> RunGet { var optionService = testLspServer.TestWorkspace.GetService(); optionService.SetGlobalOption(TaskListOptionsStorage.ComputeTaskListItemsForClosedFiles, includeTaskListItems); - await testLspServer.WaitForDiagnosticsAsync(); if (useVSDiagnostics) { - BufferedProgress? progress = useProgress ? BufferedProgress.Create(null) : null; - var diagnostics = await testLspServer.ExecuteRequestAsync( - 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> 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? progress = useProgress ? BufferedProgress.Create(null) : null; + var diagnostics = await testLspServer.ExecuteRequestAsync( + 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> RunPublicGetWorkspacePullDiagnosticsAsync( + TestLspServer testLspServer, + ImmutableArray<(string resultId, TextDocumentIdentifier identifier)>? previousResults = null, + bool useProgress = false, + bool triggerConnectionClose = true) + { + await testLspServer.WaitForDiagnosticsAsync(); + + BufferedProgress? progress = useProgress ? BufferedProgress.Create(null) : null; + var diagnosticsTask = testLspServer.ExecuteRequestAsync( + Methods.WorkspaceDiagnosticName, + CreateProposedWorkspaceDiagnosticParams(previousResults, progress), + CancellationToken.None).ConfigureAwait(false); + + if (triggerConnectionClose) { - BufferedProgress? progress = useProgress ? BufferedProgress.Create(null) : null; - var returnedResult = await testLspServer.ExecuteRequestAsync( - 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(); + 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? 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(); - return new WorkspaceDiagnosticParams - { - PreviousResultId = previousResultsLsp, - PartialResultToken = progress - }; - } + Uri = r.identifier.Uri, + Value = r.resultId + }).ToArray() ?? Array.Empty(); + return new WorkspaceDiagnosticParams + { + PreviousResultId = previousResultsLsp, + PartialResultToken = progress + }; + } - static TestDiagnosticResult ConvertWorkspaceDiagnosticResult(SumType workspaceReport) + private static TestDiagnosticResult ConvertWorkspaceDiagnosticResult(SumType 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); } } diff --git a/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs b/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs index 810b424a39a8c..71261332617df 100644 --- a/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs +++ b/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs @@ -1658,6 +1658,54 @@ public async Task TestWorkspaceDiagnosticsDoesNotThrowIfProjectWithoutFilePathEx Assert.Equal(@"C:/C2.cs", results[2].TextDocument.Uri.AbsolutePath); } + [Fact] + public async Task TestPublicWorkspaceDiagnosticsWaitsForLspTextChanges() + { + var markup1 = +@"class A {"; + var markup2 = ""; + await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync( + new[] { markup1, markup2 }, BackgroundAnalysisScope.FullSolution, useVSDiagnostics: false); + + var resultTask = RunPublicGetWorkspacePullDiagnosticsAsync(testLspServer, useProgress: true, triggerConnectionClose: false); + + // Assert that the connection isn't closed and task doesn't complete even after some delay. + await Task.Delay(TimeSpan.FromSeconds(5)); + Assert.False(resultTask.IsCompleted); + + // Make an LSP document change that will trigger connection close. + var uri = testLspServer.GetCurrentSolution().Projects.First().Documents.First().GetURI(); + await testLspServer.OpenDocumentAsync(uri); + + // Assert the task completes after a change occurs + var results = await resultTask; + Assert.NotEmpty(results); + } + + [Fact] + public async Task TestPublicWorkspaceDiagnosticsWaitsForLspSolutionChanges() + { + var markup1 = +@"class A {"; + var markup2 = ""; + await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync( + new[] { markup1, markup2 }, BackgroundAnalysisScope.FullSolution, useVSDiagnostics: false); + + var resultTask = RunPublicGetWorkspacePullDiagnosticsAsync(testLspServer, useProgress: true, triggerConnectionClose: false); + + // Assert that the connection isn't closed and task doesn't complete even after some delay. + await Task.Delay(TimeSpan.FromSeconds(5)); + Assert.False(resultTask.IsCompleted); + + // Make workspace change that will trigger connection close. + var projectInfo = testLspServer.TestWorkspace.Projects.Single().ToProjectInfo(); + testLspServer.TestWorkspace.OnProjectReloaded(projectInfo); + + // Assert the task completes after a change occurs + var results = await resultTask; + Assert.NotEmpty(results); + } + #endregion } }