Ensure NFW gets reported before result is reported to client and remo…

…ve async listener in queue (dotnet#75907)

Should resolve
https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2224584

Optprof web application tests have been failing for a while in VS. Tim
tracked down the issue to an async listener introduced in
dotnet#74530

The never completing listener were requests for workspace diagnostics.
This is actually expected for workspace diagnostics requests, as they
are held open indefinitely if nothing changes (see
https://github.com/dotnet/roslyn/blob/main/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractWorkspacePullDiagnosticsHandler.cs#L94).
So as long as workspace diagnostics requests function this way, it is
easy to get into a situation where there are async listeners that never
complete.

The queue async listener was introduced to ensure that unit tests could
reliably verify if the NFW handler was called (as handling the exception
happened after the response was returned to the client).

To resolve this issue I did a couple things
1. Move reporting NFW for request exceptions into the telemetry
reporting scope. Importantly this scope is called before the result is
sent back to the client. This allows unit tests to simply wait for the
request to complete before checking if the NFW handler was called (no
need for an async listener).
2. Removing the async listener from the queue now that no unit tests
need to wait for an action to occur in the queue.

TODO - validate optprof

src/EditorFeatures/Core/ExternalAccess/VSTypeScript/VSTypeScriptRequestExecutionQueueProvider.cs

@@ -15,11 +15,11 @@ namespace Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript;
 [ExportStatelessLspService(typeof(IRequestExecutionQueueProvider<RequestContext>), ProtocolConstants.TypeScriptLanguageContract), Shared]
 [method: ImportingConstructor]
 [method: Obsolete(MefConstruction.ImportingConstructorMessage, true)]
-internal sealed class VSTypeScriptRequestExecutionQueueProvider(IAsynchronousOperationListenerProvider asynchronousOperationListenerProvider) : IRequestExecutionQueueProvider<RequestContext>
+internal sealed class VSTypeScriptRequestExecutionQueueProvider() : IRequestExecutionQueueProvider<RequestContext>
 {
     public IRequestExecutionQueue<RequestContext> CreateRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider)
     {
-        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider, asynchronousOperationListenerProvider);
+        var queue = new RoslynRequestExecutionQueue(languageServer, logger, handlerProvider);
         queue.Start();
         return queue;
     }

src/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs

@@ -406,12 +406,12 @@ protected internal virtual void BeforeRequest<TRequest>(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.
     /// </summary>
-    /// <param name="nonMutatingRequestTask">The task to be inspected.</param>
+    /// <param name="requestTask">The task to be inspected.</param>
     /// <param name="rethrowExceptions">If exceptions should be re-thrown.</param>
-    /// <returns>The task from <paramref name="nonMutatingRequestTask"/>, to allow chained calls if needed.</returns>
-    public virtual Task WrapStartRequestTaskAsync(Task nonMutatingRequestTask, bool rethrowExceptions)
+    /// <returns>The task from <paramref name="requestTask"/>, to allow chained calls if needed.</returns>
+    public virtual Task WrapStartRequestTaskAsync(Task requestTask, bool rethrowExceptions)
     {
-        return nonMutatingRequestTask;
+        return requestTask;
     }
 
     /// <summary>

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,18 @@ 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)
+        {
+            // 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;
+        }
+
+        FatalError.ReportAndPropagateUnlessCanceled(exception, ErrorSeverity.Critical);
+    }
 }

src/LanguageServer/Protocol/RequestExecutionQueueProvider.cs

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

src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs

@@ -5,55 +5,36 @@
 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<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, IAsynchronousOperationListenerProvider provider)
+        public RoslynRequestExecutionQueue(AbstractLanguageServer<RequestContext> languageServer, ILspLogger logger, AbstractHandlerProvider handlerProvider)
             : base(languageServer, logger, handlerProvider)
         {
             _initializeManager = languageServer.GetLspServices().GetRequiredService<IInitializeManager>();
-            _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
             {
-                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
+            catch (Exception) when (!rethrowExceptions)
             {
-                // The caller has asked us to not rethrow, so record a NFW and swallow.
-                await nonMutatingRequestTask.ReportNonFatalErrorAsync().ConfigureAwait(false);
+                // 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).
             }
         }
 

src/LanguageServer/ProtocolUnitTests/HandlerTests.cs

@@ -10,7 +10,6 @@
 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;
@@ -151,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;
                 }
@@ -163,14 +162,37 @@ public async Task NonMutatingHandlerExceptionNFWIsReported(bool mutatingLspWorks
             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 NonMutatingHandlerExceptionNFWIsNotReportedForLocalRpcException(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 StreamJsonRpc.LocalRpcException(nameof(HandlerTests)) { ErrorCode = LspErrorCodes.ContentModified });
+            TestConfigurableDocumentHandler.ConfigureHandler(server, mutatesSolutionState: false, requiresLspSolution: true, response);
+
+            await Assert.ThrowsAnyAsync<Exception>(async ()
+                => await server.ExecuteRequestAsync<TestRequestWithDocument, TestConfigurableResponse>(TestConfigurableDocumentHandler.MethodName, request, CancellationToken.None));
+
+            Assert.False(didReport);
+        }
+
         [Theory, CombinatorialData]
         public async Task MutatingHandlerExceptionNFWIsReported(bool mutatingLspWorkspace)
         {
@@ -184,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;
                 }
@@ -214,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;
                 }
@@ -226,11 +248,6 @@ public async Task NonMutatingHandlerCancellationExceptionNFWIsNotReported(bool m
             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);
         }
 
@@ -247,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;
                 }