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.

Sign up for GitHub

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

Jump to bottom

Handle OOM in Java language server #548

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Handle OOM in Java language server #548

merged 1 commit into from
Aug 4, 2021

Conversation

datho7561
Copy link
Contributor

Add parameters to the java server to make lemminx crash when it runs out of memory. Detect when the java server shuts down due to running out of memory, display a message to the user that this happened, and don't attempt to restart the server.

Closes #527

Signed-off-by: David Thompson [email protected]

@datho7561 datho7561 marked this pull request as ready for review July 14, 2021 13:32
@datho7561 datho7561 requested a review from rgrunber July 14, 2021 13:32
@datho7561
Copy link
Contributor Author

@fbricon it seems like it isn't possible to do anything on the lemminx side when we hit an OOM Error. By crashing lemminx and checking for the heap dump, we can notify the user that its an OOM and prevent the server from restarting.

rgrunber
rgrunber requested changes Jul 15, 2021
View reviewed changes
Copy link
Member

@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.

This worked pretty well for me. I think it's a good first step. We might be able to improve things if the language server client had a nice way to access the exit code of the LS executable, but not possible for now it seems.

src/client/clientErrorHandler.ts Show resolved Hide resolved
src/server/java/javaServerStarter.ts Outdated Show resolved Hide resolved
docs/Troubleshooting.md Outdated Show resolved Hide resolved
@datho7561 datho7561 requested a review from rgrunber July 15, 2021 17:59
rgrunber
rgrunber reviewed Jul 19, 2021
View reviewed changes
Copy link
Member

@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.

Looks good. Just a minor detail to change.

Just wondering (whether it's possible, and worth it), if a user does set -XX:HeapDumpPath=.., could we listen to that location for newly added files ?

docs/Troubleshooting.md Outdated Show resolved Hide resolved
fbricon
fbricon reviewed Jul 19, 2021
View reviewed changes
docs/Troubleshooting.md Outdated Show resolved Hide resolved
@datho7561
Copy link
Contributor Author

Just wondering (whether it's possible, and worth it), if a user does set -XX:HeapDumpPath=.., could we listen to that location for newly added files ?

I implemented a strategy that listens to the HeapDumpPath specified by the JVM arguments if the user sets it there. Unlike when the argument is set in the typescript code, it won't delete the heap dumps that are generated, since presumably the user wants to read them.

@datho7561 datho7561 force-pushed the 527-handle-oom branch 2 times, most recently from 96e1890 to f0bf439 Compare July 19, 2021 18:55
rgrunber
rgrunber reviewed Jul 19, 2021
View reviewed changes
src/utils/contextUtils.ts Outdated Show resolved Hide resolved
@datho7561 datho7561 force-pushed the 527-handle-oom branch 2 times, most recently from 05ff7e8 to 93bac0f Compare July 19, 2021 20:01
@datho7561
Copy link
Contributor Author

@fbricon does this PR work for you? You should see a popup when the language server crashes due to an OOM Error

@datho7561 datho7561 force-pushed the 527-handle-oom branch 2 times, most recently from 21e623c to 6807407 Compare July 28, 2021 17:26
@rgrunber
Copy link
Member

I can confirm that data would be broadcast correctly upon crash, and for opening the link. @fbricon , if you're fine with this, I think we can probably merge.

@datho7561
Handle OOM in Java language server
6b7b620 
Add parameters to the java server to make lemminx crash
when it runs out of memory.
Detect when the java server shuts down due to running out of memory,
display a message to the user that this happened,
and don't attempt to restart the server.

Closes redhat-developer#527

Signed-off-by: David Thompson <[email protected]>
@rgrunber
Copy link
Member

rgrunber commented Aug 4, 2021 •
edited
Loading

I think we can merge this now.

@datho7561 datho7561 merged commit 064ac73 into redhat-developer:master Aug 4, 2021
@datho7561 datho7561 deleted the 527-handle-oom branch August 4, 2021 18:46
@rgrunber rgrunber mentioned this pull request Aug 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrunber rgrunber rgrunber approved these changes

@fbricon fbricon fbricon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

XML Language Server keeps restarting when running out of memory
3 participants
@datho7561 @rgrunber @fbricon