Ensure NFW gets reported before result is reported to client and remove async listener in queue

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

@@ -424,12 +424,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,66 +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.
-                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.
-                }
-                catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex, ErrorSeverity.Critical))
-                {
-                    // Swallow the exception so it does not bubble up into the queue.
-                }
+                // 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,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<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);
         }
 
@@ -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<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);
         }
 
@@ -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<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);
         }
 
@@ -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;
                 }