Cleanup LSP error reporting

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<RequestContext>), ProtocolConstants.TypeScriptLanguageContract), Shared]
-internal sealed class VSTypeScriptRequestExecutionQueueProvider : IRequestExecutionQueueProvider<RequestContext>
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, true)]
+internal sealed class VSTypeScriptRequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider<RequestContext>
 {
-    [ImportingConstructor]
-    [Obsolete(MefConstruction.ImportingConstructorMessage, true)]
-    public VSTypeScriptRequestExecutionQueueProvider()
-    {
-    }
-
     public IRequestExecutionQueue<RequestContext> CreateRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider)
     {
-        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider);
+        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider);
         queue.Start();
         return queue;
     }

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.
+                    }
                 }
             }
 

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<RequestContext>)), Shared]
-internal sealed class RequestExecutionQueueProvider : IRequestExecutionQueueProvider<RequestContext>
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, true)]
+internal sealed class RequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider<RequestContext>
 {
-    [ImportingConstructor]
-    [Obsolete(MefConstruction.ImportingConstructorMessage, true)]
-    public RequestExecutionQueueProvider()
-    {
-    }
-
     public IRequestExecutionQueue<RequestContext> CreateRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider)
     {
-        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider);
+        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider);
         queue.Start();
         return queue;
     }

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<RequestContext>
     {
         private readonly IInitializeManager _initializeManager;
+        private readonly IAsynchronousOperationListener _listener;
 
         /// <summary>
         /// Serial access is guaranteed by the queue.
         /// </summary>
         private CultureInfo? _cultureInfo;
 
-        public RoslynRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider)
+        public RoslynRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider, IAsynchronousOperationListenerProvider provider)
             : base(languageServer, logger, handlerProvider)
         {
             _initializeManager = languageServer.GetLspServices().GetRequiredService<IInitializeManager>();
+            _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);
             }
         }
 

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<TestConfigurableResponse>(new InvalidOperationException(nameof(HandlerTests)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: false, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<Exception>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None));
+
+            var provider = server.TestWorkspace.ExportProvider.GetExportedValue<AsynchronousOperationListenerProvider>();
+            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<TestConfigurableResponse>(new InvalidOperationException(nameof(HandlerTests)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: true, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<Exception>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(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<TestConfigurableResponse>(new OperationCanceledException(nameof(HandlerTests)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: false, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<Exception>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None));
+
+            var provider = server.TestWorkspace.ExportProvider.GetExportedValue<AsynchronousOperationListenerProvider>();
+            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<TestConfigurableResponse>(new OperationCanceledException(nameof(HandlerTests)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: true, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<Exception>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(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);

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<TestRequestWithDocument, TestConfigurableResponse>
+{
+    public const string MethodName = nameof(TestConfigurableDocumentHandler);
+
+    private bool? _mutatesSolutionState;
+    private bool? _requiresLSPSolution;
+    private Task<TestConfigurableResponse>? _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<TestConfigurableResponse> 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<TestConfigurableResponse> 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<TestConfigurableResponse> 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);
+    }
+}