From 5900c2f671c16f9d850a6d622c31f9ece964adc7 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Wed, 13 Nov 2024 16:07:58 -0800 Subject: [PATCH 1/3] Ensure NFW gets reported before result is reported to client and remove async listener in queue --- ...TypeScriptRequestExecutionQueueProvider.cs | 4 +-- .../RequestExecutionQueue.cs | 8 ++--- .../LspServices/RequestTelemetryScope.cs | 15 ++++++++ .../Protocol/RequestExecutionQueueProvider.cs | 5 ++- .../Protocol/RoslynRequestExecutionQueue.cs | 35 ++++--------------- .../ProtocolUnitTests/HandlerTests.cs | 27 +++----------- 6 files changed, 34 insertions(+), 60 deletions(-) diff --git a/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs b/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs index e2097d53c6631..d4c90787529e1 100644 --- a/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs +++ b/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs @@ -15,11 +15,11 @@ namespace Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript; [ExportStatelessLspService(typeof(IRequestExecutionQueueProvider), ProtocolConstants.TypeScriptLanguageContract), Shared] [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, true)] -internal sealed class VSTypeScriptRequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider +internal sealed class VSTypeScriptRequestExecutionQueueProvider() : IRequestExecutionQueueProvider { public IRequestExecutionQueue CreateRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider) { - var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider); + var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider); queue.Start(); return queue; } diff --git a/src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs b/src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs index 0f987aa808ff2..6da78c6e625f2 100644 --- a/src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs +++ b/src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs @@ -424,12 +424,12 @@ protected internal virtual void BeforeRequest(TRequest request) /// Provides an extensibility point to log or otherwise inspect errors thrown from non-mutating requests, /// which would otherwise be lost to the fire-and-forget task in the queue. /// - /// The task to be inspected. + /// The task to be inspected. /// If exceptions should be re-thrown. - /// The task from , to allow chained calls if needed. - public virtual Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions) + /// The task from , to allow chained calls if needed. + public virtual Task WrapStartRequestTaskAsync(Task requestTask, bool rethrowExceptions) { - return nonMutatingRequestTask; + return requestTask; } /// diff --git a/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs b/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs index 7cc25cf9e74ba..66d1071293ea7 100644 --- a/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs +++ b/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.LanguageServer.Handler; using Microsoft.CommonLanguageServerProtocol.Framework; using Roslyn.Utilities; @@ -29,6 +30,9 @@ public override void RecordCancellation() public override void RecordException(Exception exception) { + // Report a NFW report for the request failure, as well as recording statistics on the failure. + ReportNonFatalError(exception); + _result = RequestTelemetryLogger.Result.Failed; } @@ -43,4 +47,15 @@ public override void Dispose() _telemetryLogger.UpdateTelemetryData(Name, Language, _queuedDuration, requestDuration, _result); } + + private static void ReportNonFatalError(Exception exception) + { + if (exception is StreamJsonRpc.LocalRpcException localRpcException && localRpcException.ErrorCode == LspErrorCodes.ContentModified) + { + // Content modified exceptions are expected and should not be reported as NFWs. + return; + } + + FatalError.ReportAndPropagateUnlessCanceled(exception, ErrorSeverity.Critical); + } } diff --git a/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs b/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs index 66bbad16e4f66..7e39bbc1dbc40 100644 --- a/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs +++ b/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs @@ -6,7 +6,6 @@ using System.Composition; using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.LanguageServer.Handler; -using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CommonLanguageServerProtocol.Framework; namespace Microsoft.CodeAnalysis.LanguageServer; @@ -14,11 +13,11 @@ namespace Microsoft.CodeAnalysis.LanguageServer; [ExportCSharpVisualBasicStatelessLspService(typeof(IRequestExecutionQueueProvider)), Shared] [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, true)] -internal sealed class RequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider +internal sealed class RequestExecutionQueueProvider() : IRequestExecutionQueueProvider { public IRequestExecutionQueue CreateRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider) { - var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider); + var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider); queue.Start(); return queue; } diff --git a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs index e12fced425f83..c0e28138c3197 100644 --- a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs +++ b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs @@ -5,63 +5,40 @@ using System; using System.Globalization; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.LanguageServer.Handler; -using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CommonLanguageServerProtocol.Framework; -using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.LanguageServer { internal sealed class RoslynRequestExecutionQueue : RequestExecutionQueue { private readonly IInitializeManager _initializeManager; - private readonly IAsynchronousOperationListener _listener; /// /// Serial access is guaranteed by the queue. /// private CultureInfo? _cultureInfo; - public RoslynRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider, IAsynchronousOperationListenerProvider provider) + public RoslynRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider) : base(languageServer, logger, handlerProvider) { _initializeManager = languageServer.GetLspServices().GetRequiredService(); - _listener = provider.GetListener(FeatureAttribute.LanguageServer); } - public override async Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions) + public override async Task WrapStartRequestTaskAsync(Task requestTask, bool rethrowExceptions) { - using var token = _listener.BeginAsyncOperation(nameof(WrapStartRequestTaskAsync)); if (rethrowExceptions) { - try - { - await nonMutatingRequestTask.ConfigureAwait(false); - } - catch (StreamJsonRpc.LocalRpcException localRpcException) when (localRpcException.ErrorCode == LspErrorCodes.ContentModified) - { - // Content modified exceptions are expected and should not be reported as NFWs. - throw; - } - // If we had an exception, we want to record a NFW for it AND propogate it out to the queue so it can be handled appropriately. - catch (Exception ex) when (FatalError.ReportAndPropagateUnlessCanceled(ex, ErrorSeverity.Critical)) - { - throw ExceptionUtilities.Unreachable(); - } + await requestTask.ConfigureAwait(false); } else { - // The caller has asked us to not rethrow, so record a NFW and swallow. + // The caller has asked us to not rethrow, so swallow the exception. try { - await nonMutatingRequestTask.ConfigureAwait(false); - } - catch (StreamJsonRpc.LocalRpcException localRpcException) when (localRpcException.ErrorCode == LspErrorCodes.ContentModified) - { - // Content modified exceptions are expected and should not be reported as NFWs. + await requestTask.ConfigureAwait(false); } - catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex, ErrorSeverity.Critical)) + catch (Exception) { // Swallow the exception so it does not bubble up into the queue. } diff --git a/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs b/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs index 402b78d7ab003..3787a64d7a2d8 100644 --- a/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs @@ -10,14 +10,12 @@ using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.LanguageServer.Handler; -using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CodeAnalysis.Test.Utilities; using Microsoft.CommonLanguageServerProtocol.Framework; using Roslyn.LanguageServer.Protocol; using Roslyn.Test.Utilities; using Xunit; using Xunit.Abstractions; -using static Microsoft.CodeAnalysis.LanguageServer.UnitTests.LocaleTests; namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests { @@ -152,7 +150,7 @@ public async Task NonMutatingHandlerExceptionNFWIsReported(bool mutatingLspWorks var didReport = false; FatalError.OverwriteHandler((exception, severity, dumps) => { - if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + if (exception.Message == nameof(HandlerTests) || exception.InnerException?.Message == nameof(HandlerTests)) { didReport = true; } @@ -164,11 +162,6 @@ public async Task NonMutatingHandlerExceptionNFWIsReported(bool mutatingLspWorks await Assert.ThrowsAnyAsync(async () => await server.ExecuteRequestAsync(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None)); - var provider = server.TestWorkspace.ExportProvider.GetExportedValue(); - await provider.WaitAllDispatcherOperationAndTasksAsync( - server.TestWorkspace, - FeatureAttribute.LanguageServer); - Assert.True(didReport); } @@ -185,7 +178,7 @@ public async Task NonMutatingHandlerExceptionNFWIsNotReportedForLocalRpcExceptio var didReport = false; FatalError.OverwriteHandler((exception, severity, dumps) => { - if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + if (exception.Message == nameof(HandlerTests) || exception.InnerException?.Message == nameof(HandlerTests)) { didReport = true; } @@ -197,11 +190,6 @@ public async Task NonMutatingHandlerExceptionNFWIsNotReportedForLocalRpcExceptio await Assert.ThrowsAnyAsync(async () => await server.ExecuteRequestAsync(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None)); - var provider = server.TestWorkspace.ExportProvider.GetExportedValue(); - await provider.WaitAllDispatcherOperationAndTasksAsync( - server.TestWorkspace, - FeatureAttribute.LanguageServer); - Assert.False(didReport); } @@ -218,7 +206,7 @@ public async Task MutatingHandlerExceptionNFWIsReported(bool mutatingLspWorkspac var didReport = false; FatalError.OverwriteHandler((exception, severity, dumps) => { - if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + if (exception.Message == nameof(HandlerTests) || exception.InnerException?.Message == nameof(HandlerTests)) { didReport = true; } @@ -248,7 +236,7 @@ public async Task NonMutatingHandlerCancellationExceptionNFWIsNotReported(bool m var didReport = false; FatalError.OverwriteHandler((exception, severity, dumps) => { - if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + if (exception.Message == nameof(HandlerTests) || exception.InnerException?.Message == nameof(HandlerTests)) { didReport = true; } @@ -260,11 +248,6 @@ public async Task NonMutatingHandlerCancellationExceptionNFWIsNotReported(bool m await Assert.ThrowsAnyAsync(async () => await server.ExecuteRequestAsync(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None)); - var provider = server.TestWorkspace.ExportProvider.GetExportedValue(); - await provider.WaitAllDispatcherOperationAndTasksAsync( - server.TestWorkspace, - FeatureAttribute.LanguageServer); - Assert.False(didReport); } @@ -281,7 +264,7 @@ public async Task MutatingHandlerCancellationExceptionNFWIsNotReported(bool muta var didReport = false; FatalError.OverwriteHandler((exception, severity, dumps) => { - if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + if (exception.Message == nameof(HandlerTests) || exception.InnerException?.Message == nameof(HandlerTests)) { didReport = true; } From ee2d1857b02d8839405d8f4fdeae83f6213ca9d0 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Thu, 14 Nov 2024 11:52:15 -0800 Subject: [PATCH 2/3] review feedback --- .../Protocol/LspServices/RequestTelemetryScope.cs | 5 ++++- .../Protocol/RoslynRequestExecutionQueue.cs | 12 ++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs b/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs index 66d1071293ea7..2408cd4025890 100644 --- a/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs +++ b/src/LanguageServer/Protocol/LspServices/RequestTelemetryScope.cs @@ -52,7 +52,10 @@ private static void ReportNonFatalError(Exception exception) { if (exception is StreamJsonRpc.LocalRpcException localRpcException && localRpcException.ErrorCode == LspErrorCodes.ContentModified) { - // Content modified exceptions are expected and should not be reported as NFWs. + // We throw content modified exceptions when asked to resolve code lens / inlay hints associated with a solution version we no longer have. + // This generally happens when the project changes underneath us. The client is eventually told to refresh, + // but they can send us resolve requests for prior versions before they see the refresh. + // There is no need to report these exceptions as NFW since they are expected to occur in normal workflows. return; } diff --git a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs index c0e28138c3197..e8ff58d15579e 100644 --- a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs +++ b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs @@ -27,21 +27,13 @@ public RoslynRequestExecutionQueue(AbstractLanguageServer langua public override async Task WrapStartRequestTaskAsync(Task requestTask, bool rethrowExceptions) { - if (rethrowExceptions) + try { await requestTask.ConfigureAwait(false); } - else + catch (Exception) when (!rethrowExceptions) { // The caller has asked us to not rethrow, so swallow the exception. - try - { - await requestTask.ConfigureAwait(false); - } - catch (Exception) - { - // Swallow the exception so it does not bubble up into the queue. - } } } From ea7c07a125bae4a696434ab9e76d917ab5293c50 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Thu, 14 Nov 2024 11:57:50 -0800 Subject: [PATCH 3/3] adjust comment --- src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs index e8ff58d15579e..96d786c34f98c 100644 --- a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs +++ b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs @@ -33,7 +33,8 @@ public override async Task WrapStartRequestTaskAsync(Task requestTask, bool reth } catch (Exception) when (!rethrowExceptions) { - // The caller has asked us to not rethrow, so swallow the exception. + // The caller has asked us to not rethrow, so swallow the exception to avoid bringing down the queue. + // The queue item task itself already handles reporting the exception (if any). } }