Move determination of language for an LSP request into the serialized queue to avoid race by dibarbet · Pull Request #74300 · dotnet/roslyn

dibarbet · 2024-07-09T06:49:43Z

Resolves dotnet/vscode-csharp#6973
Resolves dotnet/vscode-csharp#7303

Previously we were doing deserialization and language determination in the direct StreamJsonRpc callback, before the request got added to the queue.

This caused a race when the language was provided as part of a DidOpen - the language would get saved as part of running the DidOpen request in the queue, however it is possible (likely even) that the next request hit the StreamJsonRpc callback and attempted to retrieve the language (outside of the queue) before the queue could run the prior DidOpen.

Additionally, any deserialization failures would not be observed by our exception reporting (as it also happened outside the queue).

This modifies the queue implementation to instead do all deserialization and language determination inside the serial execution part of the queue. This fixes the race as it ensures the previous DidOpen completes before the next request attempts to determine the language.

Additionally, serialization failures are now tracked. Failures in mutating requests will shut down the server, non-mutating will not.

… queue to avoid race

dibarbet · 2024-07-09T18:32:43Z

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

        {
            _method = method;
-            _typeRefResolver = typeRefResolver;
-            _languageEntryPoint = new Lazy<FrozenDictionary<string, (MethodInfo, RequestHandlerMetadata)>>(() =>


This code generally moved into the RequestExecutionQueue

dibarbet · 2024-07-09T18:34:57Z

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

-            await task.ConfigureAwait(false);
-            var resultProperty = task.GetType().GetProperty("Result") ?? throw new InvalidOperationException("Result property on task cannot be null");
-            var result = resultProperty.GetValue(task);
+            var result = await queue.ExecuteAsync(requestObject, _method, lspServices, cancellationToken).ConfigureAwait(false);


Deserialization and things moved into the queue, had to move the reflection invocation down into the queue as well

dibarbet · 2024-07-09T18:35:36Z

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

    private readonly JsonSerializer _jsonSerializer = jsonSerializer;

-    protected override DelegatingEntryPoint CreateDelegatingEntryPoint(string method, IGrouping<string, RequestHandlerMetadata> handlersForMethod)
+    public override TRequest DeserializeRequest<TRequest>(object? serializedRequest, RequestHandlerMetadata metadata)


generally just moved from further down in this file.

dibarbet · 2024-07-09T18:37:40Z

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

        return context;
    }

+    public TRequest? TryDeserializeRequest<TRequest>(AbstractLanguageServer<TRequestContext> languageServer, RequestHandlerMetadata requestHandlerMetadata, bool isMutating)


Previously, deserialization errors would never be observed by the queue, the request would just fail (but the server would continue functioning).

Now that we're deserializing in the queue, we log the errors and throw/catch based on what kind of request it is.

dibarbet · 2024-07-09T18:38:33Z

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

+    /// The handler info is created lazily to avoid instantiating any types or handlers until a request is recieved for
+    /// that particular method and language.
+    /// </summary>
+    private readonly FrozenDictionary<string, FrozenDictionary<string, Lazy<(RequestHandlerMetadata Metadata, IMethodHandler Handler, MethodInfo MethodInfo)>>> _handlerInfoMap;


Caching method infos for invoking handlers with specific parameters.

nit: i recommend the Rewrap extension, set to 120 column. Makes it easy to wrap comments so you don't have to mnually fix things up.

dibarbet · 2024-07-09T18:38:55Z

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

+            var languages = new Dictionary<string, Lazy<(RequestHandlerMetadata, IMethodHandler, MethodInfo)>>();
+            foreach (var metadata in methodGroup)
+            {
+                languages.Add(metadata.Language, new(() =>


this was migrated from AbstractLanguageServer

dibarbet · 2024-07-09T18:39:28Z

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

+
+        // 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(deserializedRequest, handler, cancellationToken).ConfigureAwait(false);


Everything below this was just moved into this method.

dibarbet · 2024-07-09T18:39:42Z

...guageServer/Microsoft.CommonLanguageServerProtocol.Framework/SystemTextJsonLanguageServer.cs

    private readonly JsonSerializerOptions _jsonSerializerOptions = options;

-    protected override DelegatingEntryPoint CreateDelegatingEntryPoint(string method, IGrouping<string, RequestHandlerMetadata> handlersForMethod)
+    public override TRequest DeserializeRequest<TRequest>(object? serializedRequest, RequestHandlerMetadata metadata)


moved from further down in the file

dibarbet · 2024-07-09T18:40:53Z

src/LanguageServer/Protocol/RoslynRequestExecutionQueue.cs

+        protected internal override void BeforeRequest<TRequest>(TRequest request)
+        {
+            // Update the locale for this request to the desired LSP locale.
+            CultureInfo.CurrentUICulture = GetCultureForRequest();


Fixing a bug that showed up after the queue changes - previously we were running this in WrapStartRequestTaskAsync, but the task could potentially be completed by the time WrapStartRequestTaskAsync was called. Instead we need to explicitly set this before the request task runs.

dibarbet · 2024-07-09T18:50:23Z

...guageServer/Microsoft.CommonLanguageServerProtocol.Framework/SystemTextJsonLanguageServer.cs

            var lspServices = target.GetLspServices();

-            // Retrieve the language of the request so we know how to deserialize it.
-            var language = target.GetLanguageForRequest(_method, request);


this was the problematic part. getting the language outside of the queue.

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

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

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

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

CyrusNajmabadi · 2024-07-09T19:04:07Z

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

+            }
+            else
+            {
+                return default;


ah, this answers teh prior question. i'd like if the prior docs can explain this.

CyrusNajmabadi · 2024-07-09T19:05:50Z

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

+            _logger.LogException(ex);
+
+            // Set the task result to the exception
+            _completionSource.TrySetException(ex);


interesting htat we set the exception like this in the failure case. but the above try doesn't do TrySetResult in the success case.

Also... this code will run in the case of cancellation. Is that desirable? Should cancellation be handled in this exact same way. If so, can you add a comment to the effect that we do want that.

note: TrySetException on cancellation is odd. I would expect TrySetCanceled.

The try doesn't set a result because it needs to do more processing to set the actual result (invoke the handler).

However if we encounter an exception, we can't continue processing so we set the result on the source so the client can observe it (the request is failed).

Also... this code will run in the case of cancellation. Is that desirable? Should cancellation be handled in this exact same way. If so, can you add a comment to the effect that we do want that.

Good question. If there is no cancellation token passed here do we need to handle that?

Good question. If there is no cancellation token passed here do we need to handle that?

probably not. but it would be good to then add asserst taht we do NOT expect to get cancellation exceptions and whatnot.

it's a little unclear to me how this all flows. For example, some people pass the CT along as a property of some context-object. I don't like that pattern because then i can't see if cancellation flows in. But i really don't have a good sense of what is going on here.

The try doesn't set a result because it needs to do more processing to set the actual result (invoke the handler).

doc the code :)

some more comments on this. will share in person though.

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

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

CyrusNajmabadi · 2024-07-09T19:12:07Z

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

+    /// Reflection invokes <see cref="ProcessQueueCoreAsync{TRequest, TResponse}(IQueueItem{TRequestContext}, IMethodHandler, RequestHandlerMetadata, ConcurrentDictionary{Task, CancellationTokenSource}, CancellationTokenSource?, CancellationToken)"/>
+    /// using the concrete types defined by the handler's metadata.
    /// </summary>
-    [Obsolete("Only for use by legacy XAML LSP")]


CyrusNajmabadi · 2024-07-09T19:13:16Z

Overall have no issue with this. Have some questions and tiny requests to cover before i sign off. but the approach makes sense :)

Mirrors dotnet/roslyn#74300 for razor fixes devdiv.visualstudio.com/DevDiv/_workitems/edit/2286513?src=WorkItemMention&src-action=artifact_link

ghost added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 9, 2024

dibarbet force-pushed the fix_lsp_language_2 branch from 6ce37ea to 50fe512 Compare July 9, 2024 07:03

Move determination of language for an LSP request into the serialized…

fdaf58d

… queue to avoid race

dibarbet force-pushed the fix_lsp_language_2 branch from 50fe512 to fdaf58d Compare July 9, 2024 18:30

dibarbet commented Jul 9, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix_lsp_language_2

3527d45

dibarbet marked this pull request as ready for review July 9, 2024 18:49

dibarbet requested a review from a team as a code owner July 9, 2024 18:49

dibarbet commented Jul 9, 2024

View reviewed changes