Skip to content

Handle errors in RequestExecutionQueue#47107

Merged
davidwengier merged 21 commits intodotnet:features/lspClassificationsfrom
davidwengier:ExecutionQueueLifetime
Sep 21, 2020
Merged

Handle errors in RequestExecutionQueue#47107
davidwengier merged 21 commits intodotnet:features/lspClassificationsfrom
davidwengier:ExecutionQueueLifetime

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Aug 25, 2020 •
edited
Loading

Fixes #46051

This PR is the "final" piece of request queue stuff, except merging the solutions.

This introduces error handling in the request queue including sending back a message to the client and disconnecting. I've tested this in Live Share and the client reconnects silently when it next needs to, and correctly calls Initialize and everything continues as expected.

This also changes the lifetime of the RequestExecutionQueue so its not a MEF component, but is directly constructed by each server. In addition the queue is used directly from the InProcLanguageServer instead of going through the AbstractRequestHandlerProvider, though it still needs to know about it.

@davidwengier davidwengier requested a review from a team as a code owner August 25, 2020 05:41
sharwell
sharwell previously requested changes Aug 25, 2020
View reviewed changes
@davidwengier davidwengier changed the title Preliminary implementation for whole document classification Handle errors in RequestExecutionQueue Aug 25, 2020
@dibarbet dibarbet mentioned this pull request Aug 25, 2020
Closed
8 tasks
CyrusNajmabadi
CyrusNajmabadi reviewed Aug 25, 2020
View reviewed changes
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs Outdated
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipped this for now.

dibarbet
dibarbet reviewed Aug 26, 2020
View reviewed changes
jasonmalinowski
jasonmalinowski requested changes Aug 27, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's one potential issue where DequeueAsync() could raise cancellation during the shutdown process and we might treat that as a reportable error even though things are good.

src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs Outdated
Copy link
Member

@jasonmalinowski jasonmalinowski Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now that we're having to do these catches anyways, does do we really need the ranToCompletion boolean as well? If instead work.CallbackAsync just returns the same task from the TaskCompletionSource it returned to the original caller, then you can just simplify this to:

try
{
   await work.CallbackAsync(....);
   lastMutatedSolution = mutatedSolution ?? lastMutatedSolution;
}
catch (OperationCanceledException)
{
}
catch (Exception e)
{
    OnRequestServerShutdown($"An error occured processing a mutating request and the solution is in an invalid state. Check LSP client logs for any error information.");
}

Which also means you now have the exception in hand if you want to pass a message back to the OnRequestServerShutdown rather than eating the exception if the logs are unavailable.

Copy link
Member Author

@davidwengier davidwengier Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could work, but I don't think I'm a big fan because the Callback will have to catch and rethrow the exceptions, and I think it will generally be harder to understand what is going on. You're right it would make for a better log message, which might be useful.

@davidwengier davidwengier force-pushed the ExecutionQueueLifetime branch from eec6398 to 2e77925 Compare August 31, 2020 04:26
@davidwengier
Copy link
Member Author

davidwengier commented Sep 2, 2020

Ping @sharwell @jasonmalinowski all feedback has been addressed, except the bool return from the callback.

jasonmalinowski
jasonmalinowski approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks OK from the threading perspective (modulo some comments), although I'm still cautious about what the merging logic is ultimately going to look like. Depending on how it ultimately works this RequestExecutionQueue implementation can be greatly simplified (I think) with the pattern we use other parts of Roslyn where we just have a queue of Tasks created with ContinueWith calls; the non-mutating in that case are just tasks that branch off and don't actually get placed in the queue. I think that approach gets us the error handling and cancellation logic automatically. But that's very much contingent on how that "TODO" comment gets resolved!

I admit the big picture of how an ILspSolutionProvider vs. AbstractRequestHandlerProvider vs. AbstractLanguageServerClient isn't clear to me, and although that's not the doing of this pull request it means I can't say with any confidence whether the changes to that structure here are right. So hopefully somebody else can confirm that bit. 😄

@davidwengier
Copy link
Member Author

davidwengier commented Sep 11, 2020

But that's very much contingent on how that "TODO" comment gets resolved!

Indeed, I was originally expecting the "merge" to be complicated but I think it might end up being relatively simple so some of this could be looked at again. We're still working out what exactly the merge will do though :)

@sharwell sharwell dismissed their stale review September 21, 2020 01:33

Issue has been addressed

@davidwengier davidwengier merged commit 0df8fee into dotnet:features/lspClassifications Sep 21, 2020
@davidwengier davidwengier deleted the ExecutionQueueLifetime branch September 21, 2020 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@dibarbet dibarbet dibarbet left review comments

@allisonchou allisonchou Awaiting requested review from allisonchou

@sharwell sharwell Awaiting requested review from sharwell

+1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@davidwengier @jasonmalinowski @sharwell @CyrusNajmabadi @dibarbet