From 63948269e74ed984745ec810facb73dfe4d8e8d8 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Thu, 1 Jul 2021 20:23:09 -0700 Subject: [PATCH 1/3] Prevent race in LSP pull diagnostics handling that causes stale diagnostics that do not update. --- .../AbstractPullDiagnosticHandler.cs | 86 +++++++++++-------- 1 file changed, 49 insertions(+), 37 deletions(-) diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index 00472af69e679..f090dc78b0b81 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -35,9 +35,9 @@ internal abstract class AbstractPullDiagnosticHandler - /// Lock to protect and . + /// Semaphore to protect and . /// - private readonly object _gate = new(); + private readonly SemaphoreSlim _gate = new(initialCount: 1); /// /// Mapping of a document to the last result id we reported for it. @@ -101,7 +101,9 @@ private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateA if (updateArgs.DocumentId == null) return; - lock (_documentIdToLastResultId) + // Ensure we do not clear the cached results while the handler is reading (and possibly then writing) + // to the cached results. + using (_gate.DisposableWait()) { // Whenever we hear about changes to a document, drop the data we've stored for it. We'll recompute it as // necessary on the next request. @@ -146,21 +148,31 @@ private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateA continue; } - if (DiagnosticsAreUnchanged(documentToPreviousDiagnosticParams, document)) - { - context.TraceInformation($"Diagnostics were unchanged for document: {document.FilePath}"); + TReport report; - // Nothing changed between the last request and this one. Report a (null-diagnostics, - // same-result-id) response to the client as that means they should just preserve the current - // diagnostics they have for this file. - var previousParams = documentToPreviousDiagnosticParams[document]; - progress.Report(CreateReport(previousParams.TextDocument, diagnostics: null, previousParams.PreviousResultId)); - } - else + // Ensure the read to _documentIdToLastResultId to get the cached resultId and subsequent write + // to _documentIdToLastResultId after calculating new diagnostics happens in a single 'transaction' + // to prevent us from overwriting data in _documentIdToLastResultId with stale results. + using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) { - context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}"); - await ComputeAndReportCurrentDiagnosticsAsync(context, progress, document, cancellationToken).ConfigureAwait(false); + if (DiagnosticsAreUnchanged(documentToPreviousDiagnosticParams, document)) + { + context.TraceInformation($"Diagnostics were unchanged for document: {document.FilePath}"); + + // Nothing changed between the last request and this one. Report a (null-diagnostics, + // same-result-id) response to the client as that means they should just preserve the current + // diagnostics they have for this file. + var previousParams = documentToPreviousDiagnosticParams[document]; + report = CreateReport(previousParams.TextDocument, diagnostics: null, previousParams.PreviousResultId); + } + else + { + context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}"); + report = await ComputeAndReportCurrentDiagnosticsAsync(context, document, cancellationToken).ConfigureAwait(false); + } } + + progress.Report(report); } // If we had a progress object, then we will have been reporting to that. Otherwise, take what we've been @@ -199,9 +211,8 @@ private static Dictionary GetDocumentToPreviousDiagn return result; } - private async Task ComputeAndReportCurrentDiagnosticsAsync( + private async Task ComputeAndReportCurrentDiagnosticsAsync( RequestContext context, - BufferedProgress progress, Document document, CancellationToken cancellationToken) { @@ -230,7 +241,7 @@ private async Task ComputeAndReportCurrentDiagnosticsAsync( result.Add(ConvertDiagnostic(document, text, diagnostic)); } - progress.Report(RecordDiagnosticReport(document, result.ToArray())); + return RecordDiagnosticReport(document, result.ToArray()); } private void HandleRemovedDocuments(RequestContext context, DiagnosticParams[] previousResults, BufferedProgress progress) @@ -257,31 +268,32 @@ private void HandleRemovedDocuments(RequestContext context, DiagnosticParams[] p } } + /// + /// Determines if diagnostics have changed since the last report based on the request resultId and + /// the resultId stored in Called under + /// private bool DiagnosticsAreUnchanged(Dictionary documentToPreviousDiagnosticParams, Document document) { - lock (_gate) - { - var workspace = document.Project.Solution.Workspace; - return documentToPreviousDiagnosticParams.TryGetValue(document, out var previousParams) && - _documentIdToLastResultId.TryGetValue((workspace, document.Id), out var lastReportedResultId) && - lastReportedResultId == previousParams.PreviousResultId; - } + var workspace = document.Project.Solution.Workspace; + return documentToPreviousDiagnosticParams.TryGetValue(document, out var previousParams) && + _documentIdToLastResultId.TryGetValue((workspace, document.Id), out var lastReportedResultId) && + lastReportedResultId == previousParams.PreviousResultId; } + /// + /// Reports diagnostics and caches report, called under + /// private TReport RecordDiagnosticReport(Document document, VSDiagnostic[] diagnostics) { - lock (_gate) - { - // Keep track of the diagnostics we reported here so that we can short-circuit producing diagnostics for - // the same diagnostic set in the future. Use a custom result-id per type (doc diagnostics or workspace - // diagnostics) so that clients of one don't errantly call into the other. For example, a client - // getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for - // doc-diagnostics. The two systems are different and cannot share results, or do things like report - // what changed between each other. - var resultId = $"{GetType().Name}:{_nextDocumentResultId++}"; - _documentIdToLastResultId[(document.Project.Solution.Workspace, document.Id)] = resultId; - return CreateReport(ProtocolConversions.DocumentToTextDocumentIdentifier(document), diagnostics, resultId); - } + // Keep track of the diagnostics we reported here so that we can short-circuit producing diagnostics for + // the same diagnostic set in the future. Use a custom result-id per type (doc diagnostics or workspace + // diagnostics) so that clients of one don't errantly call into the other. For example, a client + // getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for + // doc-diagnostics. The two systems are different and cannot share results, or do things like report + // what changed between each other. + var resultId = $"{GetType().Name}:{_nextDocumentResultId++}"; + _documentIdToLastResultId[(document.Project.Solution.Workspace, document.Id)] = resultId; + return CreateReport(ProtocolConversions.DocumentToTextDocumentIdentifier(document), diagnostics, resultId); } private VSDiagnostic ConvertDiagnostic(Document document, SourceText text, DiagnosticData diagnosticData) From a122223859e1cb2e85665bb1a6e339d6e8d644ac Mon Sep 17 00:00:00 2001 From: David Barbet Date: Fri, 2 Jul 2021 12:24:50 -0700 Subject: [PATCH 2/3] Move cache write before diagnostic computation to avoid computing under a lock --- .../AbstractPullDiagnosticHandler.cs | 102 +++++++++--------- 1 file changed, 54 insertions(+), 48 deletions(-) diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index f090dc78b0b81..12bace2970004 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Diagnostics; @@ -35,9 +36,9 @@ internal abstract class AbstractPullDiagnosticHandler - /// Semaphore to protect and . + /// Lock to protect and . /// - private readonly SemaphoreSlim _gate = new(initialCount: 1); + private readonly object _gate = new(); /// /// Mapping of a document to the last result id we reported for it. @@ -103,7 +104,7 @@ private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateA // Ensure we do not clear the cached results while the handler is reading (and possibly then writing) // to the cached results. - using (_gate.DisposableWait()) + lock (_gate) { // Whenever we hear about changes to a document, drop the data we've stored for it. We'll recompute it as // necessary on the next request. @@ -148,31 +149,21 @@ private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateA continue; } - TReport report; - - // Ensure the read to _documentIdToLastResultId to get the cached resultId and subsequent write - // to _documentIdToLastResultId after calculating new diagnostics happens in a single 'transaction' - // to prevent us from overwriting data in _documentIdToLastResultId with stale results. - using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) + if (HaveDiagnosticsChangedAndUpdateCache(documentToPreviousDiagnosticParams, document, out var newResultId)) { - if (DiagnosticsAreUnchanged(documentToPreviousDiagnosticParams, document)) - { - context.TraceInformation($"Diagnostics were unchanged for document: {document.FilePath}"); - - // Nothing changed between the last request and this one. Report a (null-diagnostics, - // same-result-id) response to the client as that means they should just preserve the current - // diagnostics they have for this file. - var previousParams = documentToPreviousDiagnosticParams[document]; - report = CreateReport(previousParams.TextDocument, diagnostics: null, previousParams.PreviousResultId); - } - else - { - context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}"); - report = await ComputeAndReportCurrentDiagnosticsAsync(context, document, cancellationToken).ConfigureAwait(false); - } + context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}"); + progress.Report(await ComputeAndReportCurrentDiagnosticsAsync(context, document, newResultId, cancellationToken).ConfigureAwait(false)); } + else + { + context.TraceInformation($"Diagnostics were unchanged for document: {document.FilePath}"); - progress.Report(report); + // Nothing changed between the last request and this one. Report a (null-diagnostics, + // same-result-id) response to the client as that means they should just preserve the current + // diagnostics they have for this file. + var previousParams = documentToPreviousDiagnosticParams[document]; + progress.Report(CreateReport(previousParams.TextDocument, diagnostics: null, previousParams.PreviousResultId)); + } } // If we had a progress object, then we will have been reporting to that. Otherwise, take what we've been @@ -214,6 +205,7 @@ private static Dictionary GetDocumentToPreviousDiagn private async Task ComputeAndReportCurrentDiagnosticsAsync( RequestContext context, Document document, + string resultId, CancellationToken cancellationToken) { // Being asked about this document for the first time. Or being asked again and we have different @@ -241,7 +233,7 @@ private async Task ComputeAndReportCurrentDiagnosticsAsync( result.Add(ConvertDiagnostic(document, text, diagnostic)); } - return RecordDiagnosticReport(document, result.ToArray()); + return CreateReport(ProtocolConversions.DocumentToTextDocumentIdentifier(document), result.ToArray(), resultId); } private void HandleRemovedDocuments(RequestContext context, DiagnosticParams[] previousResults, BufferedProgress progress) @@ -269,31 +261,45 @@ private void HandleRemovedDocuments(RequestContext context, DiagnosticParams[] p } /// - /// Determines if diagnostics have changed since the last report based on the request resultId and - /// the resultId stored in Called under + /// Returns true if diagnostics have changed since the last request and if so, + /// calculates a new resultId to use for subsequent computation and caches it. /// - private bool DiagnosticsAreUnchanged(Dictionary documentToPreviousDiagnosticParams, Document document) + /// the resultIds the client sent us. + /// the document we are currently calculating results for. + /// the resultId to report new diagnostics with if changed. + private bool HaveDiagnosticsChangedAndUpdateCache( + Dictionary documentToPreviousDiagnosticParams, + Document document, + [NotNullWhen(true)] out string? newResultId) { - var workspace = document.Project.Solution.Workspace; - return documentToPreviousDiagnosticParams.TryGetValue(document, out var previousParams) && - _documentIdToLastResultId.TryGetValue((workspace, document.Id), out var lastReportedResultId) && - lastReportedResultId == previousParams.PreviousResultId; - } + // Read and write the cached resultId to _documentIdToLastResultId in a single 'transaction' + // to prevent in-between updates to _documentIdToLastResultId triggered by OnDiagnosticsUpdated. + lock (_gate) + { + var workspace = document.Project.Solution.Workspace; + if (documentToPreviousDiagnosticParams.TryGetValue(document, out var previousParams) && + _documentIdToLastResultId.TryGetValue((workspace, document.Id), out var lastReportedResultId) && + lastReportedResultId == previousParams.PreviousResultId) + { + // Our cached resultId for the document matches the resultId the client passed to us. + // This means the diagnostics have not changed and we do not need to re-compute. + newResultId = null; + return false; + } - /// - /// Reports diagnostics and caches report, called under - /// - private TReport RecordDiagnosticReport(Document document, VSDiagnostic[] diagnostics) - { - // Keep track of the diagnostics we reported here so that we can short-circuit producing diagnostics for - // the same diagnostic set in the future. Use a custom result-id per type (doc diagnostics or workspace - // diagnostics) so that clients of one don't errantly call into the other. For example, a client - // getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for - // doc-diagnostics. The two systems are different and cannot share results, or do things like report - // what changed between each other. - var resultId = $"{GetType().Name}:{_nextDocumentResultId++}"; - _documentIdToLastResultId[(document.Project.Solution.Workspace, document.Id)] = resultId; - return CreateReport(ProtocolConversions.DocumentToTextDocumentIdentifier(document), diagnostics, resultId); + // Keep track of the diagnostics we reported here so that we can short-circuit producing diagnostics for + // the same diagnostic set in the future. Use a custom result-id per type (doc diagnostics or workspace + // diagnostics) so that clients of one don't errantly call into the other. For example, a client + // getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for + // doc-diagnostics. The two systems are different and cannot share results, or do things like report + // what changed between each other. + // + // Note that we can safely update the map before computation as any cancellation or exception + // during computation means that the client will never recieve this resultId and so cannot ask us for it. + newResultId = $"{GetType().Name}:{_nextDocumentResultId++}"; + _documentIdToLastResultId[(document.Project.Solution.Workspace, document.Id)] = newResultId; + return true; + } } private VSDiagnostic ConvertDiagnostic(Document document, SourceText text, DiagnosticData diagnosticData) From 8e101332dd12a3f623bf6fcaee9749089f33c93b Mon Sep 17 00:00:00 2001 From: David Barbet Date: Fri, 2 Jul 2021 13:22:57 -0700 Subject: [PATCH 3/3] feedback --- .../Handler/Diagnostics/AbstractPullDiagnosticHandler.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index 12bace2970004..40fdb82553285 100644 --- a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -149,7 +149,7 @@ private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateA continue; } - if (HaveDiagnosticsChangedAndUpdateCache(documentToPreviousDiagnosticParams, document, out var newResultId)) + if (HaveDiagnosticsChanged(documentToPreviousDiagnosticParams, document, out var newResultId)) { context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}"); progress.Report(await ComputeAndReportCurrentDiagnosticsAsync(context, document, newResultId, cancellationToken).ConfigureAwait(false)); @@ -267,12 +267,12 @@ private void HandleRemovedDocuments(RequestContext context, DiagnosticParams[] p /// the resultIds the client sent us. /// the document we are currently calculating results for. /// the resultId to report new diagnostics with if changed. - private bool HaveDiagnosticsChangedAndUpdateCache( + private bool HaveDiagnosticsChanged( Dictionary documentToPreviousDiagnosticParams, Document document, [NotNullWhen(true)] out string? newResultId) { - // Read and write the cached resultId to _documentIdToLastResultId in a single 'transaction' + // Read and write the cached resultId to _documentIdToLastResultId in a single transaction // to prevent in-between updates to _documentIdToLastResultId triggered by OnDiagnosticsUpdated. lock (_gate) {