Move the task completion source for a queue item into the queue item itself. by dibarbet · Pull Request #57617 · dotnet/roslyn

dibarbet · 2021-11-06T02:56:02Z

Resolves #57221

…itself.

dibarbet · 2021-11-06T02:56:56Z

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

-        private static Task ExecuteCallbackAsync(QueueItem work, RequestContext? context, CancellationToken queueCancellationToken)
-        {
-            // Create a combined cancellation token to cancel any requests in progress when this shuts down
-            using var combinedTokenSource = queueCancellationToken.CombineWith(work.CancellationToken);


handled in the work.CallbackAsync method now - the queue item is responsible for the cancellation token from the client and the queue cancellation token is passed in. Before handling the request the tokens are combined and checked for cancellation

note: an alternative formalizatino woudl be to not store hte combined token in the item, and instead have a queue of (IQueueItem, CancellationToken). They're isomorphic. But it's a question of who should morally own the CT.

CyrusNajmabadi · 2021-11-06T04:46:17Z

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

            {
+                // Create a combined cancellation token so either the client cancelling it's token or the queue
+                // shutting down cancels the request.
+                using var combinedTokenSource = queueCancellationToken.CombineWith(queueCancellationToken);


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

CyrusNajmabadi · 2021-11-06T04:56:44Z

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

+            /// A task completion source representing the result of this queue item's work.
            /// </summary>
-            public readonly Action<Exception> HandleQueueFailure;
+            public TaskCompletionSource<TResponseType?> CompletionSource { get; }


public? but not in the interface?

CyrusNajmabadi · 2021-11-06T04:56:51Z

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

    internal partial class RequestExecutionQueue
    {
-        private readonly struct QueueItem
+        private interface QueueItem


Suggested change

private interface QueueItem

private interface IQueueItem

CyrusNajmabadi · 2021-11-06T04:57:09Z

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

+            public RequestMetrics Metrics { get; }
+        }
+
+        private readonly struct QueueItem<TRequestType, TResponseType> : QueueItem


being a struct isn't really helpful here if it's going to always be used through the itnerface.

CyrusNajmabadi · 2021-11-06T05:00:12Z

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

            while (_queue.TryDequeue(out var item))
            {
-                _ = item.CallbackAsync(null, new CancellationToken(true));
+                _ = item.TrySetCanceled(_cancelSource.Token);


something feels weird here with the CTs. it may be right. but i'm not feeling confident it it. I feel like we should not be cancelling it with our token, but shuld just be asking the item to cancel itself with it's own token that tracks its work.

--

note: i'd liek to actually talk through this so I can get a concreate understanding of exactly how cancellation works here. thanks :)

sharwell · 2021-11-08T16:02:33Z

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

+            /// Begins executing the work specified by this queue item.
            /// </summary>
-            private readonly Func<RequestContext?, CancellationToken, Task> _callbackAsync;
+            public Task CallbackAsync(RequestContext? context, CancellationToken cancellationToken);


All of these access modifiers should probably be removed

Suggested change

public Task CallbackAsync(RequestContext? context, CancellationToken cancellationToken);

Task CallbackAsync(RequestContext? context, CancellationToken cancellationToken);

remind me. when do we call the callback when RequestContext is null? (feel free to doc this as well).

sharwell · 2021-11-08T16:18:28Z

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

            }

-            return completion.Task;
+            return item.CompletionSource.Task;


❓ Does this execute inside Visual Studio? Is it expected to cooperate with JTF?

it does execute in VS, however I believe it shouldn't need jtf since

JTF does not keep context across the streamjsonrpc boundary, so even if the client is blocking the UI, using JTF here won't help

LSP handlers should not be using the UI thread. If they do it is a bug. There currently one that does, but because of 1) it is broken in any scenario where the LSP server is running on the ~~same machine~~ process ([LSP] CSharpAddMissingReference.InvokeSomeFixesInCSharpThenVerifyReferences can deadlock in LSP editor #51951)

sharwell · 2021-11-08T16:23:46Z

You could probably avoid the interface by making the QueueItem constructor private and adding a generic Create<TResponse> method where callbackAsync returns Task<TResult> instead of just Task.

dibarbet · 2021-11-08T19:23:05Z

You could probably avoid the interface by making the QueueItem constructor private and adding a generic Create<TResponse> method where callbackAsync returns Task<TResult> instead of just Task.

Wouldn't it still need to be generic since the TaskCompletionSouce requires a type param? I could make it a TaskCompletionSource<object> perhaps, but I'm not sure if I prefer that over just the interface

…n queue

CyrusNajmabadi · 2021-11-09T00:17:36Z

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


            public bool IsComplete() => _queue._queue.IsCompleted && _queue._queue.IsEmpty;
+
+            public async Task<bool> AreAllItemsCancelledAsync()


this function scares me a bit. it asks a query question, but it mutates to process.

CyrusNajmabadi · 2021-11-09T00:18:07Z

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

+                while (!_queue._queue.IsEmpty)
+                {
+                    var item = await _queue._queue.DequeueAsync().ConfigureAwait(false);
+                    if (!item.CombinedCancellationToken.IsCancellationRequested)


if this is false, then the item was not canceled, but it was dequeued from teh queue... seems really strange. doc heavily if this is actualy what we want.

didn't quite get this one actually - but this was changed to cancellationToken.ThrowIfCancellationRequested(); This just exits early if the work item was cancelled while in queue so we don't even try to run it.

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

CyrusNajmabadi · 2021-11-09T00:20:38Z

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

+                        _logger.TraceInformation($"{MethodName} - Canceled while in queue");
+                        this.Metrics.RecordCancellation();
+
+                        _completionSource.SetCanceled();


pass in the CT into SetCanceled?

modified this bit to just ThrowIfCancellationRequested and let the catch below handle setting the result

CyrusNajmabadi · 2021-11-09T00:20:58Z

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

+                    // 'null' response.  Note: the lsp spec was checked to ensure that 'null' is valid for all
+                    // the requests this could happen for.  However, this assumption may not hold in the future.
+                    // If that turns out to be the case, we could defer to the individual handler to decide
+                    // what to do.


great comment.

haha I believe you wrote it :)

CyrusNajmabadi · 2021-11-09T00:23:05Z

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

+                    _completionSource.SetException(ex);
+
+                    // Also allow the exception to flow back to the request queue to handle as appropriate
+                    throw new InvalidOperationException($"Error handling '{MethodName}' request: {ex.Message}", ex);


this is def a bit... interesting. not saying it's wrong. but i always find ti a bit suspect . it would be good to have a deeper comment on why this is needed. for example, since we did _completionSource.SetException(ex); this somewhat implies that someone is not observing the TCS itself (but still is observing the direction exception thrown).

I wouldn't mind an actual case listed where that happens. note: i get this is a move, so we should keep even if you can't give a case.

This part is fairly important.

The _completionSource represents the task completion as the LSP client sees it, so we of course send the exception result back to the client.

However, that completionSource doesn't represent the result of the call inside the queue processing. By throwig the exception here as well, the queue processing in ProcessQueueAsync also sees the exception and decides (depending on if the request was mutating or not) how to handle the exception (in the case of mutating, it shuts down the server, for non-mutating it just reports a non-fatal).

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

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

CyrusNajmabadi · 2021-11-09T00:30:15Z

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

-        private static Task ExecuteCallbackAsync(QueueItem work, RequestContext? context, CancellationToken queueCancellationToken)
-        {
-            // Create a combined cancellation token to cancel any requests in progress when this shuts down
-            using var combinedTokenSource = queueCancellationToken.CombineWith(work.CancellationToken);


note: an alternative formalizatino woudl be to not store hte combined token in the item, and instead have a queue of (IQueueItem, CancellationToken). They're isomorphic. But it's a question of who should morally own the CT.

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

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

…nc return the result of the TCS

dibarbet · 2021-11-09T03:34:12Z

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

            {
+                _completionSource = new TaskCompletionSource<TResponseType?>();
+                // Set the tcs state to cancelled if the token gets cancelled outside of our callback (for example the server shutting down).
+                cancellationToken.Register(() => _completionSource.TrySetCanceled(cancellationToken));


@CyrusNajmabadi this was a bit I wasn't sure if it was correct. But basically if an exception happens before we actually start executing the work (e.g. an exception in creating the request context), then we cancel the queue token and shutdown the server.

However, the test (or client) would still be waiting on the result of the TCS which never transitions to the canceled state since the CallbackAsync method never executes. So I register an action on cancellation of the combined token to actually cancel the TCS.

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

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

CyrusNajmabadi · 2021-11-09T03:37:29Z

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

+                        // The queue's cancellation token was invoked which means we are shutting down the queue.
+                        // Exit out of the loop so we stop processing new items.
+                        return;
+                    }


note: if you don't like this try/catch, i believe @genlu is adding an extension to await this without throign cancellation. You can then just await, and tehn do a quick check/bail if the _cancelSource is canceled.

CyrusNajmabadi · 2021-11-09T03:38:22Z

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

+                // Log it, shutdown the queue, and exit the loop.
+                _logger.TraceException(ex);
+                OnRequestServerShutdown($"Error occurred processing queue in {_serverName}: {ex.Message}.");
+                return;


nice. so it worked to not have to tell the item to handle this?

yup, discussed #57617 (comment) offline

This reverts commit ef81088, reversing changes made to 431f38b.

Move the task completion source for a queue item into the queue item …

953451f

…itself.

dibarbet added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Nov 6, 2021

dibarbet requested review from CyrusNajmabadi and davidwengier November 6, 2021 02:56

dibarbet requested a review from a team as a code owner November 6, 2021 02:56

dibarbet commented Nov 6, 2021

View reviewed changes

fix docs

a2af95f

CyrusNajmabadi reviewed Nov 6, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.QueueItem.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 6, 2021

View reviewed changes

sharwell reviewed Nov 8, 2021

View reviewed changes

dibarbet force-pushed the lsp_queue_task branch from 3309450 to 43d6dc8 Compare November 8, 2021 22:21

Update tests to account for no longer manually cancelling all tasks i…

52e5570

…n queue

dibarbet force-pushed the lsp_queue_task branch from 43d6dc8 to 52e5570 Compare November 8, 2021 23:10

dibarbet added 3 commits November 8, 2021 15:18

remove unneeded method

2a3beeb

return task exception directly

fc4c1a6

adjust

bdd3d77

CyrusNajmabadi reviewed Nov 9, 2021

View reviewed changes

CyrusNajmabadi suggested changes Nov 9, 2021

View reviewed changes

Move cancellation token to tuple with queue item and make CallbackAsy…

ff7dee1

…nc return the result of the TCS

dibarbet force-pushed the lsp_queue_task branch from bc50762 to ff7dee1 Compare November 9, 2021 03:30

dibarbet commented Nov 9, 2021

View reviewed changes

remove accessibility modifiers on interface

c50cf88

CyrusNajmabadi approved these changes Nov 9, 2021

View reviewed changes

small feedback

32da120

runfoapp bot mentioned this pull request Nov 9, 2021

Roslyn failing to clean up listeners #48816

Closed

dibarbet added 3 commits November 9, 2021 11:54

Properly dispose of server in lsp push diag tests

a3e25fc

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

f425556

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

83cc5d4

dibarbet merged commit ef81088 into dotnet:main Nov 16, 2021

ghost added this to the Next milestone Nov 16, 2021

dibarbet deleted the lsp_queue_task branch November 16, 2021 00:06

allisonchou added a commit to allisonchou/roslyn that referenced this pull request Nov 17, 2021

Revert "Merge pull request dotnet#57617 from dibarbet/lsp_queue_task"

63877e6

This reverts commit ef81088, reversing changes made to 431f38b.

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

	public Task CallbackAsync(RequestContext? context, CancellationToken cancellationToken);
	Task CallbackAsync(RequestContext? context, CancellationToken cancellationToken);


		public bool IsComplete() => _queue._queue.IsCompleted && _queue._queue.IsEmpty;

		public async Task<bool> AreAllItemsCancelledAsync()

Conversation

dibarbet commented Nov 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Nov 8, 2021

Uh oh!

dibarbet commented Nov 8, 2021 • edited by sharwell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dibarbet Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dibarbet Nov 8, 2021 •

edited

Loading

dibarbet commented Nov 8, 2021 •

edited by sharwell

Loading

dibarbet Nov 9, 2021 •

edited

Loading