From bb279fdb4b76a2fde6bddc39a2c0157950723f8a Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 23 Jul 2024 16:49:44 -0700 Subject: [PATCH] Cleanup LSP error reporting --- ...TypeScriptRequestExecutionQueueProvider.cs | 13 +- .../Protocol/Handler/AbstractRefreshQueue.cs | 10 +- .../Protocol/RequestExecutionQueueProvider.cs | 13 +- .../Protocol/RoslynRequestExecutionQueue.cs | 23 +++- .../ProtocolUnitTests/HandlerTests.cs | 129 +++++++++++++++++- .../TestConfigurableDocumentHandler.cs | 67 +++++++++ 6 files changed, 233 insertions(+), 22 deletions(-) create mode 100644 src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs diff --git a/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs b/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs index 121d529769011..e2097d53c6631 100644 --- a/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs +++ b/src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs @@ -7,22 +7,19 @@ using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.LanguageServer; using Microsoft.CodeAnalysis.LanguageServer.Handler; +using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CommonLanguageServerProtocol.Framework; namespace Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript; [ExportStatelessLspService(typeof(IRequestExecutionQueueProvider), ProtocolConstants.TypeScriptLanguageContract), Shared] -internal sealed class VSTypeScriptRequestExecutionQueueProvider : IRequestExecutionQueueProvider +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, true)] +internal sealed class VSTypeScriptRequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider { - [ImportingConstructor] - [Obsolete(MefConstruction.ImportingConstructorMessage, true)] - public VSTypeScriptRequestExecutionQueueProvider() - { - } - public IRequestExecutionQueue CreateRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider) { - var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider); + var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider); queue.Start(); return queue; } diff --git a/src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs b/src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs index 4d988ebd887a8..d877472dfe24d 100644 --- a/src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs +++ b/src/LanguageServer/Protocol/Handler/AbstractRefreshQueue.cs @@ -109,7 +109,15 @@ private ValueTask FilterLspTrackedDocumentsAsync( { if (documentUri is null || !trackedDocuments.ContainsKey(documentUri)) { - return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken); + try + { + return notificationManager.SendRequestAsync(GetWorkspaceRefreshName(), cancellationToken); + } + catch (StreamJsonRpc.ConnectionLostException) + { + // It is entirely possible that we're shutting down and the connection is lost while we're trying to send a notification + // as this runs outside of the guaranteed ordering in the queue. We can safely ignore this exception. + } } } diff --git a/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs b/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs index a0d3526307fce..66bbad16e4f66 100644 --- a/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs +++ b/src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs @@ -6,22 +6,19 @@ 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; [ExportCSharpVisualBasicStatelessLspService(typeof(IRequestExecutionQueueProvider)), Shared] -internal sealed class RequestExecutionQueueProvider : IRequestExecutionQueueProvider +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, true)] +internal sealed class RequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider { - [ImportingConstructor] - [Obsolete(MefConstruction.ImportingConstructorMessage, true)] - public RequestExecutionQueueProvider() - { - } - public IRequestExecutionQueue CreateRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider) { - var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider); + var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider); queue.Start(); return queue; } diff --git a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs index 72796e0783a89..35c7dc41e322e 100644 --- a/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs +++ b/src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs @@ -2,9 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +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; @@ -13,27 +16,39 @@ 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) + public RoslynRequestExecutionQueue(AbstractLanguageServer languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider, IAsynchronousOperationListenerProvider provider) : base(languageServer, logger, handlerProvider) { _initializeManager = languageServer.GetLspServices().GetRequiredService(); + _listener = provider.GetListener(FeatureAttribute.LanguageServer); } - public override Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions) + public override async Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions) { + using var token = _listener.BeginAsyncOperation(nameof(WrapStartRequestTaskAsync)); if (rethrowExceptions) { - return nonMutatingRequestTask; + try + { + await nonMutatingRequestTask.ConfigureAwait(false); + } + // 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(); + } } else { - return nonMutatingRequestTask.ReportNonFatalErrorAsync(); + // The caller has asked us to not rethrow, so record a NFW and swallow. + await nonMutatingRequestTask.ReportNonFatalErrorAsync().ConfigureAwait(false); } } diff --git a/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs b/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs index 86d9c2446c33e..71c0dce7b5aaa 100644 --- a/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/HandlerTests.cs @@ -7,8 +7,10 @@ using System.Text.Json.Serialization; using System.Threading; using System.Threading.Tasks; +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; @@ -32,7 +34,8 @@ public HandlerTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) typeof(TestNotificationHandlerFactory), typeof(TestNotificationWithoutParamsHandlerFactory), typeof(TestLanguageSpecificHandler), - typeof(TestLanguageSpecificHandlerWithDifferentParams)); + typeof(TestLanguageSpecificHandlerWithDifferentParams), + typeof(TestConfigurableDocumentHandler)); [Theory, CombinatorialData] public async Task CanExecuteRequestHandler(bool mutatingLspWorkspace) @@ -135,6 +138,130 @@ public async Task ShutsdownIfDeserializationFailsOnMutatingRequest(bool mutating await server.AssertServerShuttingDownAsync(); } + [Theory, CombinatorialData] + public async Task NonMutatingHandlerExceptionNFWIsReported(bool mutatingLspWorkspace) + { + await using var server = await CreateTestLspServerAsync("", mutatingLspWorkspace); + + var request = new TestRequestWithDocument(new TextDocumentIdentifier + { + Uri = ProtocolConversions.CreateAbsoluteUri(@"C:\test.cs") + }); + + var didReport = false; + FatalError.OverwriteHandler((exception, severity, dumps) => + { + if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + { + didReport = true; + } + }); + + var response = Task.FromException(new InvalidOperationException(nameof(HandlerTests))); + TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: false, requiresLspSolution: true, response); + + 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); + } + + [Theory, CombinatorialData] + public async Task MutatingHandlerExceptionNFWIsReported(bool mutatingLspWorkspace) + { + var server = await CreateTestLspServerAsync("", mutatingLspWorkspace); + + var request = new TestRequestWithDocument(new TextDocumentIdentifier + { + Uri = ProtocolConversions.CreateAbsoluteUri(@"C:\test.cs") + }); + + var didReport = false; + FatalError.OverwriteHandler((exception, severity, dumps) => + { + if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + { + didReport = true; + } + }); + + var response = Task.FromException(new InvalidOperationException(nameof(HandlerTests))); + TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: true, requiresLspSolution: true, response); + + await Assert.ThrowsAnyAsync(async () + => await server.ExecuteRequestAsync(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None)); + + await server.AssertServerShuttingDownAsync(); + + Assert.True(didReport); + } + + [Theory, CombinatorialData] + public async Task NonMutatingHandlerCancellationExceptionNFWIsNotReported(bool mutatingLspWorkspace) + { + await using var server = await CreateTestLspServerAsync("", mutatingLspWorkspace); + + var request = new TestRequestWithDocument(new TextDocumentIdentifier + { + Uri = ProtocolConversions.CreateAbsoluteUri(@"C:\test.cs") + }); + + var didReport = false; + FatalError.OverwriteHandler((exception, severity, dumps) => + { + if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + { + didReport = true; + } + }); + + var response = Task.FromException(new OperationCanceledException(nameof(HandlerTests))); + TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: false, requiresLspSolution: true, response); + + 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); + } + + [Theory, CombinatorialData] + public async Task MutatingHandlerCancellationExceptionNFWIsNotReported(bool mutatingLspWorkspace) + { + await using var server = await CreateTestLspServerAsync("", mutatingLspWorkspace); + + var request = new TestRequestWithDocument(new TextDocumentIdentifier + { + Uri = ProtocolConversions.CreateAbsoluteUri(@"C:\test.cs") + }); + + var didReport = false; + FatalError.OverwriteHandler((exception, severity, dumps) => + { + if (exception.Message == nameof(HandlerTests) || exception.InnerException.Message == nameof(HandlerTests)) + { + didReport = true; + } + }); + + var response = Task.FromException(new OperationCanceledException(nameof(HandlerTests))); + TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: true, requiresLspSolution: true, response); + + await Assert.ThrowsAnyAsync(async () + => await server.ExecuteRequestAsync(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None)); + + Assert.False(didReport); + } + internal record TestRequestTypeOne([property: JsonPropertyName("textDocument"), JsonRequired] TextDocumentIdentifier TextDocumentIdentifier); internal record TestRequestTypeTwo([property: JsonPropertyName("textDocument"), JsonRequired] TextDocumentIdentifier TextDocumentIdentifier); diff --git a/src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs b/src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs new file mode 100644 index 0000000000000..30c853f44cc77 --- /dev/null +++ b/src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs @@ -0,0 +1,67 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Composition; +using System.Text.Json.Serialization; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.LanguageServer.Handler; +using Microsoft.CommonLanguageServerProtocol.Framework; +using Roslyn.LanguageServer.Protocol; +using Roslyn.Utilities; +using static Roslyn.Test.Utilities.AbstractLanguageServerProtocolTests; + +namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests; + +internal record TestRequestWithDocument([property: JsonPropertyName("textDocument"), JsonRequired] TextDocumentIdentifier TextDocumentIdentifier); + +internal record TestConfigurableResponse([property: JsonPropertyName("response"), JsonRequired] string Response); + +[ExportCSharpVisualBasicStatelessLspService(typeof(TestConfigurableDocumentHandler)), PartNotDiscoverable, Shared] +[LanguageServerEndpoint(MethodName, LanguageServerConstants.DefaultLanguageName)] +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] +internal sealed class TestConfigurableDocumentHandler() : ILspServiceDocumentRequestHandler +{ + public const string MethodName = nameof(TestConfigurableDocumentHandler); + + private bool? _mutatesSolutionState; + private bool? _requiresLSPSolution; + private Task? _response; + + public bool MutatesSolutionState => _mutatesSolutionState ?? throw new InvalidOperationException($"{nameof(ConfigureHandler)} has not been called"); + public bool RequiresLSPSolution => _requiresLSPSolution ?? throw new InvalidOperationException($"{nameof(ConfigureHandler)} has not been called"); + + public void ConfigureHandler(bool mutatesSolutionState, bool requiresLspSolution, Task response) + { + if (_mutatesSolutionState is not null || _requiresLSPSolution is not null || _response is not null) + { + throw new InvalidOperationException($"{nameof(ConfigureHandler)} has already been called"); + } + + _mutatesSolutionState = mutatesSolutionState; + _requiresLSPSolution = requiresLspSolution; + _response = response; + } + + public TextDocumentIdentifier GetTextDocumentIdentifier(TestRequestWithDocument request) + { + return request.TextDocumentIdentifier; + } + + public Task HandleRequestAsync(TestRequestWithDocument request, RequestContext context, CancellationToken cancellationToken) + { + Contract.ThrowIfNull(_response, $"{nameof(ConfigureHandler)} has not been called"); + return _response; + } + + public static void ConfigureHandler(TestLspServer server, bool mutatesSolutionState, bool requiresLspSolution, Task response) + { + var handler = (TestConfigurableDocumentHandler)server.GetQueueAccessor()!.Value.GetHandlerProvider().GetMethodHandler(TestConfigurableDocumentHandler.MethodName, + TypeRef.From(typeof(TestRequestWithDocument)), TypeRef.From(typeof(TestConfigurableResponse)), LanguageServerConstants.DefaultLanguageName); + handler.ConfigureHandler(mutatesSolutionState, requiresLspSolution, response); + } +}