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

SapMachine #1901: Feature Request to allow service to crash if direct memory is OOM #1916

Merged
merged 8 commits into from
Feb 11, 2025
Merged

SapMachine #1901: Feature Request to allow service to crash if direct memory is OOM #1916

merged 8 commits into from
Feb 11, 2025

Conversation

ansteiner
Copy link
Member

If the direct memory buffer run into OOM, DirectByteBuffer allocation code can notify the jvm if the system property "jdk.nio.reportErrorOnDirectMemoryOom" is set.

Benefits:

  • In a containerized environment, using Crash/ExitOnOutOfMemoryError allows a app/JVM that can no longer provide services and application code is handling the OOM of DirectByteBuffer to crash/exit and enabling the service to restart.
  • To analyze direct memory OutOfMemoryErrors, heap dumps are necessary to examine reference relationships. This can be facilitated using the HeapDumpOnOutOfMemoryError option.

Implementation with some minor adaptions from closed issue/PR from OpenJDK: openjdk/jdk#16176

fixes #1901

@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

RealCLanger
RealCLanger requested changes Feb 6, 2025
View reviewed changes
Copy link
Member

@RealCLanger RealCLanger 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 overall. I made a few suggestions. Also, the new test is failing because the VM seems to exit with a different rc.

@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

@RealCLanger
Copy link
Member

OK, we need to figure out what happens in the test.

schmelter-sap
schmelter-sap reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@schmelter-sap schmelter-sap left a comment

Choose a reason for hiding this comment

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

The type of the entry is wrong.

@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

@MBaesken
Copy link
Member

Hi Andreas, have you figured out the strange exit/return codes seen in some tests ?

@ansteiner
adapt test to run with -XX:MaxDirectMemorySize=10m to fix failing tes…
a240848 
…t on macos-aarch64 due to different MaxHeapsize/MaxDirectMemorySize by ergonomics
@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

@ansteiner
Copy link
Member Author

Hi Andreas, have you figured out the strange exit/return codes seen in some tests ?

Yes, it was the wrong entry type(92a964d). Mentioned by Ralf.

ansteiner
ansteiner commented Feb 11, 2025
View reviewed changes
Copy link
Member Author

@ansteiner ansteiner left a comment

Choose a reason for hiding this comment

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

modified already

ansteiner
ansteiner commented Feb 11, 2025
View reviewed changes
Copy link
Member Author

@ansteiner ansteiner left a comment

Choose a reason for hiding this comment

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

modified already

RealCLanger
RealCLanger approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@RealCLanger RealCLanger 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 to me now. I have a few last minor formatting suggestions for the test.

@MBaesken
Copy link
Member

MBaesken commented Feb 11, 2025 •
edited
Loading

One question / remark - the system property 'jdk.nio.reportErrorOnDirectMemoryOom' is SapMachine specific but sounds like a standard property available in all JDK. Is this good? Should we make it clearer in the name that it is specific ?

But I am okay with this name as well so just an idea/remark .

@SapMachine
Copy link
Member

Hello @ansteiner, this pull request fulfills all formal requirements.

@ansteiner ansteiner merged commit 1ce231f into SAP:sapmachine Feb 11, 2025
66 of 67 checks passed
@ansteiner ansteiner deleted the direct_memory_oom branch February 11, 2025 15:09
ansteiner added a commit to ansteiner/SapMachine that referenced this pull request Feb 12, 2025
@ansteiner
SapMachine SAP#1901: Feature Request to allow service to crash if dir…
bdee71b 
…ect memory is OOM (SAP#1916)

SapMachine SAP#1901: Feature Request to allow service to crash if direct memory is OOM

(cherry picked from commit 1ce231f)
ansteiner added a commit to ansteiner/SapMachine that referenced this pull request Feb 12, 2025
@ansteiner
SapMachine SAP#1901: Feature Request to allow service to crash if dir…
4eb2d44 
…ect memory is OOM (SAP#1916)

SapMachine SAP#1901: Feature Request to allow service to crash if direct memory is OOM

(cherry picked from commit 1ce231f)
@ansteiner ansteiner self-assigned this Feb 12, 2025
RealCLanger pushed a commit that referenced this pull request Feb 13, 2025
@ansteiner @RealCLanger
SapMachine #1901: Feature Request to allow service to crash if direct…
6e9d587 
… memory is OOM (#1916)

SapMachine #1901: Feature Request to allow service to crash if direct memory is OOM

(cherry picked from commit 1ce231f)
RealCLanger pushed a commit that referenced this pull request Feb 13, 2025
@ansteiner @RealCLanger
SapMachine #1901: Feature Request to allow service to crash if direct…
cac2d6a 
… memory is OOM (#1916)

SapMachine #1901: Feature Request to allow service to crash if direct memory is OOM

(cherry picked from commit 1ce231f)
RealCLanger pushed a commit that referenced this pull request Feb 13, 2025
@ansteiner @RealCLanger
SapMachine #1901: Feature Request to allow service to crash if direct…
7eaf00a 
… memory is OOM (#1916)

SapMachine #1901: Feature Request to allow service to crash if direct memory is OOM

(cherry picked from commit 1ce231f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schmelter-sap schmelter-sap schmelter-sap left review comments

@MBaesken MBaesken MBaesken approved these changes

@RealCLanger RealCLanger RealCLanger approved these changes

Assignees

@ansteiner ansteiner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request to allow service to crash if direct memory is OOM
5 participants
@ansteiner @SapMachine @RealCLanger @MBaesken @schmelter-sap