PR feedback

src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs

@@ -27,7 +27,7 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler
     /// <para>
     /// This class acheives this by distinguishing between mutating and non-mutating requests, and ensuring that
     /// when a mutating request comes in, its processing blocks all subsequent requests. As each request comes in
-    /// it is added to a queue, and a queue item will not be retreived while a mutating request is running. Before
+    /// it is added to a queue, and a queue item will not be retrieved while a mutating request is running. Before
     /// any request is handled the solution state is created by merging workspace solution state, which could have
     /// changes from non-LSP means (eg, adding a project reference), with the current "mutated" state.
     /// When a non-mutating work item is retrieved from the queue, it is given the current solution state, but then
@@ -55,6 +55,10 @@ internal partial class RequestExecutionQueue
         private readonly CancellationTokenSource _cancelSource;
         private readonly DocumentChangeTracker _documentChangeTracker;
 
+        // These two dictionaries are used to cache our forked LSP solution so we don't have to
+        // recompute it for each request. We don't need to worry about threading because they are only
+        // used when preparing to handle a request, which happens in a single thread in the ProcessQueueAsync
+        // method.
         private readonly Dictionary<Workspace, Solution> _lastWorkspaceSolution = new();
         private readonly Dictionary<Workspace, Solution> _lastLSPSolution = new();
 
@@ -184,16 +188,14 @@ private async Task ProcessQueueAsync()
                         // Mutating requests block other requests from starting to ensure an up to date snapshot is used.
                         var ranToCompletion = await work.CallbackAsync(context, cancellationToken).ConfigureAwait(false);
 
-                        if (ranToCompletion)
-                        {
-                            // We've had a successfully mutating request, so clear out our saved state to ensure its recalculated
-                            _lastLSPSolution.Remove(context.Solution.Workspace);
-                        }
-                        else
+                        // Now that we've mutated our solution, clear out our saved state to ensure it gets recalculated
+                        _lastLSPSolution.Remove(context.Solution.Workspace);
+
+                        if (!ranToCompletion)
                         {
                             // If the handling of the request failed, the exception will bubble back up to the caller, and we
                             // request shutdown because we're in an invalid state
-                            OnRequestServerShutdown($"An error occured processing a mutating request and the solution is in an invalid state. Check LSP client logs for any error information.");
+                            OnRequestServerShutdown($"An error occurred processing a mutating request and the solution is in an invalid state. Check LSP client logs for any error information.");
                             break;
                         }
                     }
@@ -213,7 +215,7 @@ private async Task ProcessQueueAsync()
             }
             catch (Exception e) when (FatalError.ReportAndCatch(e))
             {
-                OnRequestServerShutdown($"Error occured processing queue: {e.Message}.");
+                OnRequestServerShutdown($"Error occurred processing queue: {e.Message}.");
             }
         }
 
@@ -264,6 +266,25 @@ private RequestContext CreateRequestContext(QueueItem queueItem)
             // solution
             var (documentId, workspaceSolution) = _solutionProvider.GetDocumentAndSolution(queueItem.TextDocument, queueItem.ClientName);
 
+            var lspSolution = GetLSPSolution(workspaceSolution);
+
+                        // If we got a document id back, we pull it out of our updated solution so the handler is operating on the latest
+            // document text. If document id is null here, this will just return null
+            var document = lspSolution.GetDocument(documentId);
+
+            var trackerToUse = queueItem.MutatesSolutionState
+                ? (IDocumentChangeTracker)_documentChangeTracker
+                : new NonMutatingDocumentChangeTracker(_documentChangeTracker);
+
+            return new RequestContext(lspSolution, queueItem.ClientCapabilities, queueItem.ClientName, document, trackerToUse);
+        }
+
+        /// <summary>
+        /// Gets the "LSP view of the world", either by forking the workspace solution and updating the documents we track
+        /// or by simply returning our cached solution if it is still valid.
+        /// </summary>
+        private Solution GetLSPSolution(Solution workspaceSolution)
+        {
             var workspace = workspaceSolution.Workspace;
 
             var recalculateSolution = false;
@@ -294,18 +315,7 @@ private RequestContext CreateRequestContext(QueueItem queueItem)
                 lspSolution = _lastLSPSolution[workspace];
             }
 
-            // If we got a document id back, we pull it out of our updated solution so the handler is operating on the latest
-            // document text. If document id is null here, this will just return null
-            var document = lspSolution.GetDocument(documentId);
-
-            // Logically, if a mutating request fails we don't want to take its mutations, so giving it the "real"
-            // tracker is a bad idea, but since we tear down the queue for any error anyway, the document tracker
-            // will be emptied and no future requests will be handled, so we don't need to do anything special here.
-            var trackerToUse = queueItem.MutatesSolutionState
-                ? (IDocumentChangeTracker)_documentChangeTracker
-                : new NonMutatingDocumentChangeTracker(_documentChangeTracker);
-
-            return new RequestContext(lspSolution, queueItem.ClientCapabilities, queueItem.ClientName, document, trackerToUse);
+            return lspSolution;
         }
 
         /// <summary>

src/Features/LanguageServer/ProtocolUnitTests/Ordering/RequestOrderingTests.cs

@@ -183,6 +183,8 @@ public async Task NonMutatingRequestsOperateOnTheSameSolutionAfterMutation()
             solution = await GetLSPSolution(NonMutatingRequestHandler.MethodName);
             Assert.Equal(expectedSolution, solution);
 
+            return;
+
             async Task<Solution> GetLSPSolution(string methodName)
             {
                 var request = new TestRequest(methodName);