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

Gracefully handle OutOfMemory errors #2085

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

rgrunber
Copy link
Member

@rgrunber rgrunber commented Aug 25, 2021

  • Detect and report java.lang.OutOfMemory errors
  • Fixes Explicitly report OutOfMemory error to user #1959
  • Use -XX:+ExitOnOutOfMemoryError to ensure Java language server exits
    when an OutOfMemory error occurs (rather than staying up)
  • Use -XX:+HeapDumpOnOutOfMemoryError & -XX:HeapDumpPath to generate a
    heap dump whose existence can notify the client that an OutOfMemory
    error has occured
  • Once OutOfMemory error is detected, clean up the heap dumps

Taken from redhat-developer/vscode-xml#548

A few limitations of this approach :

  • We rely particularly on setting -XX:HeapDumpPath (we set it to global storage path of the extension) in order to detect if OOM occured. If the user overrides this value, we can't really be sure.
  • The status bar icon continues to spin even after we handle the issue, but I believe this has always been a problem when a crash occurs
  • We emit the error message for the 2 servers (syntax, standard) though I haven't noticed any obvious downside to this

Signed-off-by: Roland Grunberg [email protected]

@testforstephen
Copy link
Collaborator

testforstephen commented Sep 1, 2021

Use -XX:+ExitOnOutOfMemoryError to ensure Java language server exits
when an OutOfMemory error occurs (rather than staying up)

I'm thinking if it's too strict to shutdown JVM if any OOM error is detected.

With the Java debugger (which is on top of JLS), there are cases where it may cause an OOM, it just fails the operation but not breaking the language server. See the issue microsoft/vscode-java-debug#1044

@rgrunber
Copy link
Member Author

You're right. I was finally able to reproduce this. It helped to set java.debug.settings.vmArgs to something rather high, while keeping java.jdt.ls.vmargs rather low. The result being, that the user application continues to run while the language server (currently doing debugging work) encounters more memory pressure.

I think I wrongly assumed that when java.lang.OutOfMemory is hit, the LS cannot recover, but that's only true when the thread having trouble is related to the operating of the LS. If it's something like computing some debug values, the LS continues to respond after.

A few modifications we could make :

  • Avoid using -XX:+ExitOnOutOfMemoryError. We can have a file watcher on the client side that simply detects if a heapdump was created, and if so, shows the same notifications, and if an OOM did happen, it doesn't restart if the LS ever happens to close.
  • Only show the OOM notification once per extension lifecycle. Any more might just be annoying.

Copy link
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

  • Avoid using -XX:+ExitOnOutOfMemoryError. We can have a file watcher on the client side that simply detects if a heapdump was created, and if so, shows the same notifications, and if an OOM did happen, it doesn't restart if the LS ever happens to close.
  • Only show the OOM notification once per extension lifecycle. Any more might just be annoying.

Agree, using file watcher to monitor the creation of the heapdump file is a sensible way to detect OOM error. Once an OOM error is detected, pop up a notification and provide suggestion to mitigate it.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@rgrunber rgrunber force-pushed the fix-1959 branch 3 times, most recently from d6ef672 to ab0e2d1 Compare November 14, 2021 22:14
@rgrunber rgrunber marked this pull request as ready for review November 14, 2021 22:15
Copy link
Collaborator

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

It works well with the latest commit. Only a minor comment about auto deletion of dump file, once it is addressed, feel free to merge it.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@rgrunber
Copy link
Member Author

Just a note about an additional change I made. I removed the notification that warns we won't delete heap dump files when the user sets their own -XX:HeapDumpPath=.... The reason for that warning was because the file system api could only tell us of the existence of files on some path. If a user pointed to a path that already had heap dump files, we would incorrectly detect them and assume a crash had occurred. With chokidar, we can actually be notified on file creation, so this problem goes away. We do need ignoreInitial: true to avoid the initial addition.

All that's left is whether we want to inform the user that we won't delete their heap dumps but will delete ours. I think this is the right thing to do anyways, and since we should probably avoid too many notifications, I've removed it. I've also added a check to ensure we don't add any of the heap dump options to the syntax server.

- Detect and report java.lang.OutOfMemory errors
- Fixes redhat-developer#1959
- Use -XX:+HeapDumpOnOutOfMemoryError & -XX:HeapDumpPath to generate a
  heap dump whose existence can notify the client that an OutOfMemory
  error has occured
- Use chokidar library to listen for heap dump files
- Once OutOfMemory error is detected, clean up the heap dumps (only if
  extension configured the location), and offer to increase JVM XmX
  value

Signed-off-by: Roland Grunberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explicitly report OutOfMemory error to user
2 participants