Bug ID: JDK-8285638 Unexpected change of behavior of ForkJoin common pool clearing thread locals

Type: Bug
Component: core-libs
Sub-Component: java.util.concurrent
Affected Version: 17,18

Priority: P4
Status: Open
Resolution: Unresolved

Submitted: 2022-04-26
Updated: 2024-05-02

Other
tbdUnresolved

As part of https://bugs.openjdk.java.net/browse/JDK-8246585 the ForkJoin common pool has changed it behavior wrt ThreadLocals

The change introduced makes the common pool clear the ThreadLocals since the common pool is now created using InnocuousForkJoinWorkerThread.

Is this change intended?

We found this when using logstash-logback-encoder together with the common pool - https://github.com/logfellow/logstash-logback-encoder/issues/722

It would seem like what causes https://bugs.openjdk.org/browse/JDK-8329701 is also what fixed this issue. I think we need to discuss the direction to take.
02-05-2024
Just as an FYI, one of my Spotify colleagues did some investigations into other OSS libraries that might run into problems, for example Guava and Protobuf: https://github.com/krka/commonpool-threadlocal#known-affected-libraries
11-05-2022
I added this to jsr166 refresh CR : https://bugs.openjdk.java.net/browse/JDK-8277090
01-05-2022
OK. Considering that the javadocs already say something about this case, it should be expanded. From: * In addition, if a {@link SecurityManager} is present, then * the common pool uses a factory supplying threads that have no * {@link Permissions} enabled. to ... * In addition, if a {@link SecurityManager} is present, then * the common pool uses a factory supplying threads that have no * {@link Permissions} enabled, and are not guaranteed to preserve * the values of {@link java.lang.ThreadLocal} variables across tasks. *
29-04-2022
From what I understand, thread pools can use innocuous (clearing threadlocals) or non-innocuous workqueues? To expect the common pool to behave in an non-innocuous manner might be a case of hyrums law yes (and maybe the pre-17 behaviour was even a bug), especially with the docs saying that the threads are reclaimed (https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java#L76) But since you can expect threadlocals to be preserved between tasks for threadpools you create yourself, the fact that the common pool does not might be worth documenting more explicitly?
29-04-2022
On further consideration, I don't think any action is needed. This is a case of Hyrum's law (https://www.hyrumslaw.com/) that we should resist: As one of an infinite number of things that are not guaranteed, thread locals need not persist across tasks. And even if they do, effects cannot be depended on because a different thread could run each task, or a previous task could have cleared them before finishing. These also hold for other jdk ExecutorServices, so I suppose we could add a note to this effect in ExecutorService interface, but I'm not sure this would be helpful.
29-04-2022
OK, sounds like this is just a doc clarification. Is this really just an @implNote (non-normative) or should it be part of the specification that TLs do not persist between tasks?
28-04-2022
I've always considered it the job of the security folk (who have access to non-public discussions) to also take on the task of changing the spec for things like InnocuousForkJoinWorkerThread. Or working with Doug to make it happen. Loom team must also have considered the general problem of isolating thread state between tasks.
27-04-2022
Anyone want to assign this to themselves? Thanks.
26-04-2022
I agree with Alan that we should clarify this in docs, This change was intentional, as one of several security improvements for common pool threads, that are similar to those elsewhere in JDK11+.
26-04-2022
It may be useful to expand the implNote in the class description to document that TLs do not persist after a task completes.
26-04-2022
[~dl] and/or [~martin] can you comment on this please.
26-04-2022