8355938: Addressed rare lost unpark bug 8074773 by pre-loading LockSupport.class

[kabutz]

In 2015, Google discovered a rare disastrous classloading bug in first call to LockSupport.park() that occurred in the AppClassLoader using the Java 7 ConcurrentHashMap, which used ReentrantLock for the synchronization.

Since then, the recommended fix for this bug seems to be this code snippet in any class that directly calls LockSupport.park():

static {
    // Prevent rare disastrous classloading in first call to LockSupport.park.
    // See: https://bugs.openjdk.java.net/browse/JDK-8074773
    Class<?> ensureLoaded = LockSupport.class;
}

Since the bug was logged, we have seen new classes in the JDK that call LockSupport.park(), but that do not have this code snippet. For example:

sun.nio.ch.Poller
jdk.internal.misc.ThreadFlock
java.util.concurrent.ThreadPerTaskExecutor

It should probably also be in:

java.util.concurrent.Exchanger

Considering how hard this bug is to detect and solve, it would probably be safer to add the code snippet into these newer classes.

---

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

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8355938

Issue

• JDK-8355938: Preload LockSupport.class in classes that use LockSupport (Bug - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24952

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kabutz! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@kabutz The following labels will be automatically applied to this pull request:

• core-libs
• nio

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

[mlbridge]

Webrevs

• 00: Full (71a7ec1b)

[AlanBateman]

Might be a bit premature to change these classes. I think the starting point is to page in the issue from 2015 so there is a clearer picture of where the parking permit is consumed. Was it always CHM used by parallel capable class loaders? It might be that we should preload LockSupport in initPhase3 but I think it requires more details on the original issue before doing anything.

[kabutz]

What is the potential downside of adding the static block to make sure the class is loaded?

[AlanBateman]

What is the potential downside of adding the static block to make sure the class is loaded?

LockSupport is part of the standard API, we can't force every usage to preload this class. So if we have to change anything then it would be better to do it in one place, maybe System.initPhase3. But I think we first need to page in some of the details from 10 years ago.

[liach]

The bug description seems like it is a fault in the JVM implementation - if that is the case, a core library bypass is unreliable, as such faults might happen to other classes and cause other consequences; and we might need to fix it from the VM runtime or compiler side, as the original report implies.

[AlanBateman]

The bug description seems like it is a fault in the JVM implementation - if that is the case, a core library bypass is unreliable, as such faults might happen to other classes and cause other consequences; and we might need to fix it from the VM runtime or compiler side, as the original report implies.

It seems to about nested parking that can arise with the first use of LockSupport.park from a class with a defining class loader that is not the boot loader. The first usage, say an invokestatic to call the park method, will call the loadClass on caller's defining class loader. For the app class loader, and many other custom class loaders, that are parallel capable, then there will be a CHM to support the mapping of class names to locks. Contention on the CHM seems to have lead to the nested park. CHM and a lot of other code has changed since and not clear that it will duplicate easily now. Martin's ParkLoops test from the 2015 issue is in the test tree but it might be that a variant of this that uses a custom class loader that overrides getClassLoadingLock or parks in loadClass might be able to trigger it. As a custom class loader's loadClass can do anything then it almost feels like creating a class loader needs it have it recorded immediately as an initiating class loader. Hopefully David will remember more of this when he gets back.

[viktorklang-ora]

I'll second @AlanBateman 's sentiments here. I think a systemic issue needs a wholesale solution, otherwise it'll just lead to endless sprinkling of eager initializations.

[AlanBateman]

I'll second @AlanBateman 's sentiments here. I think a systemic issue needs a wholesale solution, otherwise it'll just lead to endless sprinkling of eager initializations.

Right, and even then, the issue is deeper than this. It's when the reference to LockSupport is from a class with a non-null defining class loader. In that scenario, the non-null class loader is the initiating class loader and any parking in the delegation chain to get to the boot loader could potentially consume the park permit. I don't think we should go down the road of supporting nested parking, instead it may be that we need some eager setup. I think we can close this PR for now as it doesn't really address the issue (LockSupport is already loaded before any user code executes).

[dholmes-ora]

Hopefully David will remember more of this when he gets back.

Sadly I have to page in all the details again like everyone else.

The underlying issue is nested-parking as Alan notes. From re-reading 8074773, pre-loading in any class loaded by the boot loader, has no affect on the bug. The issue was the action taken by the AppLoader that involved creating an entry in the lockMap CHM. That particular bug-path was closed when CHM was rewritten to use object monitors (did it re-open again when Loom came along?).

[AlanBateman]

The underlying issue is nested-parking as Alan notes. From re-reading 8074773, pre-loading in any class loaded by the boot loader, has no affect on the bug. The issue was the action taken by the AppLoader that involved creating an entry in the lockMap CHM. That particular bug-path was closed when CHM was rewritten to use object monitors (did it re-open again when Loom came along?).

No, it will still uses synchronized if there is contention and this will not consume the park permit when it's a virtual thread. Im not sure if Heinz ran into an issue, or just remember the issue from 2015, Heinz?

[kabutz]

No, it will still uses synchronized if there is contention and this will not consume the park permit when it's a virtual thread. Im not sure if Heinz ran into an issue, or just remember the issue from 2015, Heinz?

I saw this comment in the JavaDoc of LockSupport:

jdk/src/java.base/share/classes/java/util/concurrent/locks/LockSupport.java

Line 135 in 7ae52ce

* // Reduce the risk of "lost unpark" due to classloading

I then searched through the JDK classes and the only ones that used LockSupport and that did not have the static {var clazz = LockSupport.class;} were the newer classes that arrived with Java 19, plus also Exchanger (which may have been an oversight).

If this is no longer an issue, and we are 100% sure of that, then we can perhaps change the example to not have that static loader?

I have not tried to reproduce the bug, and from what Martin Buchholz described it is extremely elusive.

[bridgekeeper]

@kabutz This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@kabutz This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.