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

Prevent sending shutdown job progress report #2622

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

JessicaJHee
Copy link
Contributor

Relating to redhat-developer/vscode-java#2586, the status icon could continue spinning after restarting the language server since the shutdown job is never completed.

This change ensures that the progress report for the shutdown job is never sent to avoid this issue.

Signed-off-by: Jessica He <jhe@redhat.com>
@rgrunber
Copy link
Contributor

retest this please.

Nice. I think this is the right approach! The client can't wait for the notifications from shutdown, because the shutdown job could complete in some cases after the client has shut off. The nicer solution is to avoid notifying the client that shutdown has begun with setSystem(true), which we do in a few other places.

@rgrunber

This comment was marked as outdated.

1 similar comment
@rgrunber
Copy link
Contributor

retest this please.

@rgrunber rgrunber added this to the End May 2023 milestone Apr 27, 2023
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Makes sense to merge. The shutdown job should not report anything back to the client because we cannot guarantee it can report it has completed.

In case you were wondering why we don't need to do the same at https://github.com/eclipse/eclipse.jdt.ls/blob/3fa7fbd9b904a61ff55720c2df7a270e13551c12/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/syntaxserver/SyntaxLanguageServer.java#L151 . The Syntax server doesn't register a progress report manager which sends notifications back to the client for jobs.

@rgrunber rgrunber added the bug label Apr 27, 2023
@rgrunber rgrunber merged commit 147a6a3 into eclipse-jdtls:master Apr 27, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants