Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate cancelling delegate commands #2415

Closed
datho7561 opened this issue Jan 23, 2023 · 8 comments
Closed

Investigate cancelling delegate commands #2415

datho7561 opened this issue Jan 23, 2023 · 8 comments

Comments

@datho7561
Copy link
Contributor

For extensions on to jdt.ls, such as lsp4mp, it would be nice to be able to cancel DelegateCommands. The use case for lsp4mp is cancelling the workspace symbol calculation, which lsp4mp does using JDT through a delegate command. The language server that calls the JDT command can receive the cancel request from the client, but it can't tell JDT to stop the calculation.

@rgrunber
Copy link
Contributor

rgrunber commented Jan 30, 2023

Cancelling relies on the client sending the correct protocol given some event. For example :

  • Searching for workspace symbols by typing various characters, <escape>, or an updated query signals cancellation
  • Hovering over various tokens, hovering away from the token triggers cancellation
  • Activating the type hierarchy view, <escape> or re-activating could trigger cancellation
  • Triggering completion in a file, <escape> or any change of focus triggers cancellation

It's basically up to the client to determine what causes cancellation and I see that the delegate command does have a reference to a progress monitor that was probably created from the cancel checker. Do we have an instance of some UI element in a client that correctly calls a long running delegate command that fails to respond to the cancellation ? If so we could definitely investigate that first.

@datho7561
Copy link
Contributor Author

It looks like functionality to support cancelling delegate commands exists. If you take a look at https://github.com/redhat-developer/vscode-java/blob/master/src/extension.ts#L270 you can see that it tries to read the last parameter as the cancellation token. In order to cancel, all we need to do is send a cancellation notification to jdt.ls that contains the cancellation token. I'll take a look to see if a command already exists for that, if not, it shouldn't be hard to implement.

@rgrunber
Copy link
Contributor

rgrunber commented Feb 6, 2023

If you insert a breakpoint at :

https://github.com/redhat-developer/vscode-java/blob/8476bef8501e67716c8e129dec1410c19d700951/src/typeHierarchy/typeHierarchyTree.ts#L43

and

https://github.com/eclipse/eclipse.jdt.ls/blob/71a797ad1030d5ddff0717f93036a490d497c8b7/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/commands/TypeHierarchyCommand.java#L48

Now just trigger type hierarchy on some element. Allow the client side to continue, and once the breakpoint is hit on the language server side, activate type hierarchy on a different element. This should trigger the breakpoint on the client side again. Step over it, so the cancel() runs. Now I'd expect to see the server side monitor.isCancelled() return true, but it doesn't 😐 .

This makes me wonder if it would work at all.

Update: It works just fine for type hierarchy. My mistake from above is that I shouldn't have inserted a breakpoint to step over the cancellation on the client side. I suspect it was affecting the cancellation request being sent off, as I wasn't even seeing that. Letting the client side run while triggering the type hierarchy re-calculation had the LS monitor from the previous request switch to isCancelled() being true.

@datho7561
Copy link
Contributor Author

Try it with the gradle version upgrade feature (i.e. you have a very old gradle wrapper set, and there is a popup to update the version used). I was able to get cancelling to work for that case:

  1. Add debug breakpoints at the beginning and end of the method that handles the delegate command
  2. Add some code to eclipse.jdt.ls to actually check the monitor and return early right before updating the Properties file on disk
  3. Debug eclipse.jdt.ls with a project with an old gradle wrapper version set, wait for the first debug point to hit, then click the cancel button in the popup.
  4. If you resume the debug, it shouldn't hit the second debug breakpoint

@rgrunber
Copy link
Contributor

rgrunber commented Feb 8, 2023

Ok, so then as part of this change, I think we just need to look at delegate commands that could run for a longer time, and ensure we check for cancellation. As a bonus, we could look on the vscode-java side to see if the UI supports cancelling some of these. This could be a relatively simple change to implement now that you've confirmed it should work.

@datho7561
Copy link
Contributor Author

I can submit a PR for the gradle wrapper update, but maybe someone else in the future could investigate handling other cases? I'd be happy to answer any questions about it.

datho7561 added a commit to datho7561/eclipse.jdt.ls that referenced this issue Feb 8, 2023
Check if the request to upgrade gradle is cancelled,
right before making any changes to the gradle build files.

See eclipse-jdtls#2415

Signed-off-by: David Thompson <davthomp@redhat.com>
rgrunber pushed a commit that referenced this issue Feb 8, 2023
Check if the request to upgrade gradle is cancelled,
right before making any changes to the gradle build files.

See #2415

Signed-off-by: David Thompson <davthomp@redhat.com>
@jasonhy-wang
Copy link

Working on this issue

@rgrunber
Copy link
Contributor

I think we can close this. As long as we check whether a given delegate command could reasonably be expected to receive cancellation from the client, we should provide monitor.isCancelled(). For now, we can handle these on a case by case basis.

@rgrunber rgrunber added this to the Early April 2023 milestone Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants