From 3ea7daa2d1053731fd2322f9e4d310cfe49af4e1 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Fri, 30 Sep 2022 13:46:46 -0700 Subject: [PATCH] Fix bug where request contexts were not created serially after clasp migration --- .../AbstractLanguageServerProtocolTests.cs | 2 +- .../AbstractLanguageServer.cs | 4 ++-- .../IQueueItem.cs | 11 ++++++++++- .../IRequestContextFactory.cs | 1 + .../QueueItem.cs | 13 +++++++++---- .../RequestExecutionQueue.cs | 7 +++++-- .../Ordering/RequestOrderingTests.cs | 1 - 7 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs b/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs index 0700a0855a612..9fa64d171a674 100644 --- a/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs +++ b/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs @@ -671,7 +671,7 @@ public async ValueTask DisposeAsync() // Some tests manually call shutdown, so avoid calling shutdown twice if already called. if (!LanguageServer.HasShutdownStarted) { - await LanguageServer.GetTestAccessor().ShutdownServerAsync(); + await LanguageServer.GetTestAccessor().ShutdownServerAsync("Disposing of test lsp server"); } await LanguageServer.GetTestAccessor().ExitServerAsync(); diff --git a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs index 8737aaa635769..0416323020c18 100644 --- a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs +++ b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/AbstractLanguageServer.cs @@ -293,9 +293,9 @@ internal TestAccessor(AbstractLanguageServer server) internal bool HasShutdownStarted() => _server.HasShutdownStarted; - internal Task ShutdownServerAsync() + internal Task ShutdownServerAsync(string message = "Shutting down") { - return _server.ShutdownAsync(); + return _server.ShutdownAsync(message); } internal Task ExitServerAsync() diff --git a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IQueueItem.cs b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IQueueItem.cs index 6ff28f6d22973..95236f4417770 100644 --- a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IQueueItem.cs +++ b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IQueueItem.cs @@ -17,9 +17,18 @@ public interface IQueueItem /// /// Executes the work specified by this queue item. /// + /// the context created by /// /// A which completes when the request has finished. - Task StartRequestAsync(CancellationToken cancellationToken); + Task StartRequestAsync(TRequestContext? requestContext, CancellationToken cancellationToken); + + /// + /// Creates the context that is sent to the handler for this queue item. + /// Note - this method is always called serially inside the queue before + /// running the actual request in + /// Throwing in this method will cause the server to shutdown. + /// + Task CreateRequestContextAsync(CancellationToken cancellationToken); ILspServices LspServices { get; } diff --git a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IRequestContextFactory.cs b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IRequestContextFactory.cs index bcc209e44abd9..1d065ef677d52 100644 --- a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IRequestContextFactory.cs +++ b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/IRequestContextFactory.cs @@ -15,6 +15,7 @@ public interface IRequestContextFactory { /// /// Create a object from the given . + /// Note - throwing in the implementation of this method will cause the server to shutdown. /// /// The from which to create a request. /// The request parameters. diff --git a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/QueueItem.cs b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/QueueItem.cs index 92f696d278e3f..1a7049af95c7e 100644 --- a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/QueueItem.cs +++ b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/QueueItem.cs @@ -85,6 +85,14 @@ public static (IQueueItem, Task) Create( return (queueItem, queueItem._completionSource.Task); } + public async Task CreateRequestContextAsync(CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + var requestContextFactory = LspServices.GetRequiredService>(); + var context = await requestContextFactory.CreateRequestContextAsync(this, _request, cancellationToken).ConfigureAwait(false); + return context; + } + /// /// Processes the queued request. Exceptions will be sent to the task completion source /// representing the task that the client is waiting for, then re-thrown so that @@ -92,16 +100,13 @@ public static (IQueueItem, Task) Create( /// /// /// The result of the request. - public async Task StartRequestAsync(CancellationToken cancellationToken) + public async Task StartRequestAsync(TRequestContext? context, CancellationToken cancellationToken) { _logger.LogStartContext($"{MethodName}"); try { cancellationToken.ThrowIfCancellationRequested(); - var requestContextFactory = LspServices.GetRequiredService>(); - var context = await requestContextFactory.CreateRequestContextAsync(this, _request, cancellationToken).ConfigureAwait(false); - if (context is null) { // If we weren't able to get a corresponding context for this request (for example, we diff --git a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs index ce4db32caf760..83505e233b6a8 100644 --- a/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs +++ b/src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs @@ -188,11 +188,14 @@ private async Task ProcessQueueAsync() var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken, cancellationToken); + // The request context must be created serially inside the queue to so that requests always run + // on the correct snapshot as of the last request. + var context = await work.CreateRequestContextAsync(cancellationToken).ConfigureAwait(false); if (work.MutatesServerState) { // Mutating requests block other requests from starting to ensure an up to date snapshot is used. // Since we're explicitly awaiting exceptions to mutating requests will bubble up here. - await work.StartRequestAsync(cancellationToken).ConfigureAwait(false); + await work.StartRequestAsync(context, cancellationToken).ConfigureAwait(false); } else { @@ -201,7 +204,7 @@ private async Task ProcessQueueAsync() // via NFW, though these errors don't put us into a bad state as far as the rest of the queue goes. // Furthermore we use Task.Run here to protect ourselves against synchronous execution of work // blocking the request queue for longer periods of time (it enforces parallelizabilty). - _ = Task.Run(() => work.StartRequestAsync(cancellationToken), cancellationToken); + _ = Task.Run(() => work.StartRequestAsync(context, cancellationToken), cancellationToken); } } catch (OperationCanceledException ex) when (ex.CancellationToken == queueItem.cancellationToken) diff --git a/src/Features/LanguageServer/ProtocolUnitTests/Ordering/RequestOrderingTests.cs b/src/Features/LanguageServer/ProtocolUnitTests/Ordering/RequestOrderingTests.cs index 32865ef8aed7e..3419f5fefc745 100644 --- a/src/Features/LanguageServer/ProtocolUnitTests/Ordering/RequestOrderingTests.cs +++ b/src/Features/LanguageServer/ProtocolUnitTests/Ordering/RequestOrderingTests.cs @@ -7,7 +7,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Shared.TestHooks; using Microsoft.CodeAnalysis.Test.Utilities; using Microsoft.VisualStudio.LanguageServer.Protocol; using Roslyn.Test.Utilities;