Cleanup LSP error reporting

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/RoslynRequestExecutionQueue.cs

@@ -2,8 +2,10 @@
 // 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.CommonLanguageServerProtocol.Framework;
 using Roslyn.Utilities;
@@ -25,15 +27,24 @@ public RoslynRequestExecutionQueue(AbstractLanguageServer<RequestContext> langua
             _initializeManager = languageServer.GetLspServices().GetRequiredService<IInitializeManager>();
         }
 
-        public override Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions)
+        public override async Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions)
         {
             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,6 +7,7 @@
 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.Test.Utilities;
@@ -32,7 +33,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 +137,62 @@ public async Task ShutsdownIfDeserializationFailsOnMutatingRequest(bool mutating
             await server.AssertServerShuttingDownAsync();
         }
 
+        [Theory, CombinatorialData]
+        public async Task HandlerExceptionNFWIsReported(bool mutatingLspWorkspace, bool isMutatingHandler)
+        {
+            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(HandlerExceptionNFWIsReported) || exception.InnerException.Message == nameof(HandlerExceptionNFWIsReported))
+                {
+                    didReport = true;
+                }
+            });
+
+            var response = Task.FromException<TestConfigurableResponse>(new InvalidOperationException(nameof(HandlerExceptionNFWIsReported)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: isMutatingHandler, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<StreamJsonRpc.RemoteInvocationException>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None));
+
+            Assert.True(didReport);
+        }
+
+        [Theory, CombinatorialData]
+        public async Task HandlerCancellationExceptionNFWIsNotReported(bool mutatingLspWorkspace, bool isMutatingHandler)
+        {
+            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(HandlerCancellationExceptionNFWIsNotReported) || exception.InnerException.Message == nameof(HandlerCancellationExceptionNFWIsNotReported))
+                {
+                    didReport = true;
+                }
+            });
+
+            var response = Task.FromException<TestConfigurableResponse>(new OperationCanceledException(nameof(HandlerCancellationExceptionNFWIsNotReported)));
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: isMutatingHandler, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<TaskCanceledException>(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);
+    }
+}