From 10dea9df2494a154deda1769a4337339a71dc6ad Mon Sep 17 00:00:00 2001 From: David Barbet Date: Mon, 13 Feb 2023 18:18:20 -0800 Subject: [PATCH] WIP --- .../AbstractPullDiagnosticHandler.cs | 18 +++++++ ...icWorkspacePullDiagnosticHandlerFactory.cs | 6 ++- .../PublicWorkspacePullDiagnosticsHandler.cs | 50 +++++++++++++++++++ .../Workspaces/LspWorkspaceManager.cs | 8 +++ 4 files changed, 81 insertions(+), 1 deletion(-) diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index 7f6a19db9c498..ed0a7ba5ef027 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -113,6 +113,19 @@ 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. + /// + /// TODO - must be called at the end before closing the connection otherwise we'll have a stale request context. + /// + /// + /// + protected virtual Task WaitForChangesAsync() + { + return Task.CompletedTask; + } + public async Task HandleRequestAsync( TDiagnosticsParams diagnosticsParams, RequestContext context, CancellationToken cancellationToken) { @@ -182,6 +195,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().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..e467d41695f7a 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticHandlerFactory.cs @@ -32,5 +32,9 @@ public PublicWorkspacePullDiagnosticHandlerFactory( } public ILspService CreateILspService(LspServices lspServices, WellKnownLspServerKinds serverKind) - => new PublicWorkspacePullDiagnosticsHandler(_analyzerService, _editAndContinueDiagnosticUpdateSource, _globalOptions); + { + var workspaceManager = lspServices.GetRequiredService(); + var workspaceRegistration = lspServices.GetRequiredService(); + return new PublicWorkspacePullDiagnosticsHandler(workspaceManager, workspaceRegistration, _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..a4dca8141bc0a 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; @@ -23,12 +24,31 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public; [Method(Methods.WorkspaceDiagnosticName)] internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler { + 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 = 1; + 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; + } /// @@ -118,4 +138,34 @@ 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() + { + // 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)).ConfigureAwait(false); + } + + // We've hit a change, so we close the current request. + return; + } } 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;