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

8066859: java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died #9427

Closed
wants to merge 11 commits into from

Conversation

DougLea
Copy link
Contributor

@DougLea DougLea commented Jul 8, 2022

8066859 : java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8066859: java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9427/head:pull/9427
$ git checkout pull/9427

Update a local copy of the PR:
$ git checkout pull/9427
$ git pull https://git.openjdk.org/jdk pull/9427/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9427

View PR using the GUI difftool:
$ git pr show -t 9427

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9427.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 8, 2022

👋 Welcome back dl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jul 8, 2022

@DougLea this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8066859
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 8, 2022
@openjdk
Copy link

openjdk bot commented Jul 8, 2022

@DougLea The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 8, 2022
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jul 8, 2022
@DougLea
Copy link
Contributor Author

DougLea commented Jul 8, 2022

Thanks to @AlanBateman for suggesting to disable possibly misleading stack traces in pre-allocated exceptions; now updated

@DougLea
Copy link
Contributor Author

DougLea commented Jul 8, 2022

I keep trying but failing to change commit message to something jcheck likes?

@AlanBateman
Copy link
Contributor

AlanBateman commented Jul 9, 2022

I keep trying but failing to change commit message to something jcheck likes?

There should be an "Edit" button in the top right that allows you to edit the title and change it to "8066859 : java/lang/ref/OOMEInReferenceHandler.java failed ...". There are two Skara commands that may be useful here.
Adding a comment "/issue JDK-8066859" will link the PR to this JBS issue. There is also a "/summary" command to edit the commit message.

@DougLea
Copy link
Contributor Author

DougLea commented Jul 9, 2022

/issue JDK-8066859

@openjdk openjdk bot changed the title Jdk 8066859 8066859: java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died Jul 9, 2022
@openjdk
Copy link

openjdk bot commented Jul 9, 2022

@DougLea The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

@@ -271,14 +293,17 @@ final int acquire(Node node, long arg, boolean shared,
Thread current = Thread.currentThread();
byte spins = 0, postSpins = 0; // retries upon unpark of first thread
boolean interrupted = false, first = false;
Node pred = null; // predecessor of node when enqueued
Node pred = null, t; // predecessor of node when enqueued
Copy link
Member

@dholmes-ora dholmes-ora Jul 11, 2022

Choose a reason for hiding this comment

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

Nit: please don't use this style of multi-variable declaration as they are very easy to mis-read. Also the comment only applies to one of the variables.

Copy link
Contributor Author

@DougLea DougLea Jul 11, 2022

Choose a reason for hiding this comment

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

OK, changed.

}
}
if (!isHeldExclusively() || !release(savedState = getState()))
throw LockSupport.staticIllegalMonitorStateException; // OOM
Copy link
Member

@dholmes-ora dholmes-ora Jul 11, 2022

Choose a reason for hiding this comment

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

How is it possible to get IMSE this deep into the code? And the comment is confusing - OOM?

Copy link
Contributor Author

@DougLea DougLea Jul 11, 2022

Choose a reason for hiding this comment

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

Clarified to:
// fall through if encountered OutOfMemoryError
if (!isHeldExclusively() || !release(savedState = getState()))
throw LockSupport.staticIllegalMonitorStateException;

/**
* Preallocated exceptions thrown if acquiring or releasing locks
* when OutOfMemory.
*/
Copy link
Member

@dholmes-ora dholmes-ora Jul 11, 2022

Choose a reason for hiding this comment

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

I don't see why this should be necessary. IMSE is thrown before any state changes occur and so it is is fine if the IMSE is replaced by OOME.

Even IE should be safe at the points it is thrown.

Also in both cases you want to see a full and proper stacktrace.

Copy link
Member

@dholmes-ora dholmes-ora Jul 11, 2022

Choose a reason for hiding this comment

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

Consider this another way, any place you have a throw x it must be safe to propagate that exception. It doesn't matter if that is actually x or an OOME caused by creating x.

Copy link
Contributor Author

@DougLea DougLea Jul 13, 2022

Choose a reason for hiding this comment

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

On further consideration, I agree. The very low probability that an IE or IMSE would enable further handling is not worth the extra startup cost of establishing static exceptions. Updated.

@DougLea
Copy link
Contributor Author

DougLea commented Jul 11, 2022

The idea here is that a non-OOME-throwing condition wait might be required for (some) memory to be reclaimed. This is not too farfetched for InterruptedException (if serving as a cancellation), so we allow it to be thrown (instead of OOME) in hopes that a catch of IE will free memory. It IS farfetched to think this might be the case for IllegalMonitorStateException, but I did it the same way for consistency.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 11, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 11, 2022

Webrevs

@@ -495,7 +495,7 @@ java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java 8151492 generic-
java/lang/invoke/LFCaching/LFGarbageCollectedTest.java 8078602 generic-all
java/lang/invoke/lambda/LambdaFileEncodingSerialization.java 8249079 linux-x64
java/lang/invoke/RicochetTest.java 8251969 generic-all
java/lang/ref/OOMEInReferenceHandler.java 8066859 generic-all
java/lang/CompressExpandTest.java 8287851 generic-all
Copy link
Contributor

@AlanBateman AlanBateman Jul 12, 2022

Choose a reason for hiding this comment

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

There may be merge issue here. JDK-8287851 was fixed last week and the CompressExpandTest.java removed from the exclude list at the same time.

Copy link
Contributor Author

@DougLea DougLea Jul 13, 2022

Choose a reason for hiding this comment

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

Thanks. fixed.

@@ -0,0 +1,107 @@
//package concurrent;
Copy link
Contributor

@AlanBateman AlanBateman Jul 13, 2022

Choose a reason for hiding this comment

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

Do you mind adding a copyright header to the test?

Copy link
Contributor Author

@DougLea DougLea Jul 13, 2022

Choose a reason for hiding this comment

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

OK, done.

@openjdk
Copy link

openjdk bot commented Jul 13, 2022

@DougLea This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8066859: java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 43 new commits pushed to the master branch:

  • 2583feb: 8290023: Remove use of IgnoreUnrecognizedVMOptions in gc tests
  • 44fb92e: 8290197: test/jdk/java/nio/file/Files/probeContentType/Basic.java fails on some systems for the ".rar" extension
  • f528124: 8289284: jdk.tracePinnedThreads output confusing when pinned due to native frame
  • 572c14e: 8288624: Cleanup CommentHelper.getText0
  • 6e18883: 8290162: Reset recursion counter missed in fix of JDK-8224267
  • 31f7fc0: 8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field
  • d9ca438: Merge
  • 3164c98: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM
  • c3806b9: 8289709: fatal error: stuck in JvmtiVTMSTransitionDisabler::disable_VTMS_transitions
  • 39715f3: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
  • ... and 33 more: https://git.openjdk.org/jdk/compare/0c37008917789e7b631b5c18e6f54454b1bfe038...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 13, 2022
@DougLea
Copy link
Contributor Author

DougLea commented Jul 13, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jul 13, 2022

Going to push as commit 5358045.
Since your change was applied there have been 43 commits pushed to the master branch:

  • 2583feb: 8290023: Remove use of IgnoreUnrecognizedVMOptions in gc tests
  • 44fb92e: 8290197: test/jdk/java/nio/file/Files/probeContentType/Basic.java fails on some systems for the ".rar" extension
  • f528124: 8289284: jdk.tracePinnedThreads output confusing when pinned due to native frame
  • 572c14e: 8288624: Cleanup CommentHelper.getText0
  • 6e18883: 8290162: Reset recursion counter missed in fix of JDK-8224267
  • 31f7fc0: 8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field
  • d9ca438: Merge
  • 3164c98: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM
  • c3806b9: 8289709: fatal error: stuck in JvmtiVTMSTransitionDisabler::disable_VTMS_transitions
  • 39715f3: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
  • ... and 33 more: https://git.openjdk.org/jdk/compare/0c37008917789e7b631b5c18e6f54454b1bfe038...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 13, 2022
@openjdk openjdk bot closed this Jul 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 13, 2022
@openjdk
Copy link

openjdk bot commented Jul 13, 2022

@DougLea Pushed as commit 5358045.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@DougLea DougLea deleted the JDK-8066859 branch Jul 13, 2022
// fall through if encountered OutOfMemoryError
if (!isHeldExclusively() || !release(savedState = getState()))
throw new IllegalMonitorStateException();
Copy link
Member

@dholmes-ora dholmes-ora Jul 13, 2022

Choose a reason for hiding this comment

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

I still don't see how an OOME can possibly indicate we might somehow be in a state where we need to throw IMSE!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants