JDK-8172726 : ForkJoin common pool retains a reference to the thread context class loader
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 7,8,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2017-01-09
  • Updated: 2020-07-22
  • Resolved: 2017-02-07
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 9
10Fixed 9 b156Fixed
Related Reports
Relates :  
JDK-8176551 - testCommonPoolThreadContextClassLoader fails with "Should throw SecurityException"
Relates :  
JDK-8249846 - Change of behavior after JDK-8237117: Better ForkJoinPool behavior
Relates :  
JDK-8157570 - sun.rmi.transport.GC retains a strong reference to the context class loader
Sub Tasks
JDK-8184335 :  
Release Note: Fork/Join common pool threads return the system class loader as their thread context class loader - Closed
Description
FULL PRODUCT VERSION :
java version "1.8.0_112"
Java(TM) SE Runtime Environment (build 1.8.0_112-b15)
Java HotSpot(TM) 64-Bit Server VM (build 25.112-b15, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows [Version 6.1.7601] (fully patched)

A DESCRIPTION OF THE PROBLEM :
There are multiple issues with ForkJoinPool.commonPool

1. The default thread pool does not change Thread.contextClassLoader to the system class loader. When ForkJoinPool.commonPool is used in a multi-class loader environment (e.g. a JavaEE container) this causes references to be retained to the class loader associated with the code that first uses the pool. This is highly likely to cause a memory leak.

2. The InnocuousForkJoinWorkerThread does not modify Thread.contextClassLoader so undesirable class loader references are retained even when using the InnocuousForkJoinWorkerThreadFactory

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Execute this test case:

https://github.com/markt-asf/memory-leaks/blob/master/src/org/apache/markt/leaks/concurrent/ForkJoinThreadLeak.java

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
The console should report no leaks detected.
ACTUAL -
The console reports that a memory leak is detected and use of a profiler confirms the leak.

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
https://github.com/markt-asf/memory-leaks/blob/master/src/org/apache/markt/leaks/concurrent/ForkJoinThreadLeak.java
---------- END SOURCE ----------
Comments
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/common-pool/
31-01-2017
Still distracted, but still trying to get something out today ...
30-01-2017
Although somewhat of a lesser issue, do we want to update the TCCL in ThreadPerTaskExecutor, for consistency?
26-01-2017
Chris, I'm distracted with other things, but yes, I'll try to get this done today.
24-01-2017
@Martin, are you ok to commit this to the 166 CVS, and then we can bring it into OpenJDK?
24-01-2017
Something like: /* * @test * @bug 8172726 * @summary common pool threads should not retain the current thread's TCCL * @run main/othervm ThreadContextClassLoader * @run main/othervm -DwithSecurityManager ThreadContextClassLoader */ import java.util.concurrent.ForkJoinTask; import java.util.concurrent.Phaser; import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.concurrent.ForkJoinPool.commonPool; public class ThreadContextClassLoader { public static void main(String[] args) throws Exception { Phaser phaser = new Phaser(2); // The main thread has its TCCL set to the App class loader, so set it // to something else so that can be checked Thread.currentThread().setContextClassLoader(new MarkerClassLoader()); if (System.getProperty("withSecurityManager") != null) System.setSecurityManager(new SecurityManager()); ForkJoinTask<ClassLoader> c = commonPool().submit(() -> { System.out.println(Thread.currentThread()); phaser.arrive(); return Thread.currentThread().getContextClassLoader(); }); phaser.arriveAndAwaitAdvance(); ClassLoader cl = c.get(30, SECONDS); if (cl instanceof MarkerClassLoader) throw new RuntimeException("Unexpected TCCL: " + cl); } static class MarkerClassLoader extends ClassLoader { } }
19-01-2017
This sort of code is typically undertested since it requires a security manager to provoke the creation of innocuous threads and private access is needed to check that Thread fields get nulled out. Is there existing work to build on, or should we write yet another Martin-style WhiteBox.java ?
19-01-2017
Thanks, all. I propose I integrate a fix in an upcoming jsr166 integration. Here's my attempt: (I de-lambda-fied the code, fearing bootstrap problems; I refactored the creation of ACCs) I worry about shared state in any fork join thread, not just innocuous ones. There is a case for resetting tccl after each top level task run. Index: ForkJoinPool.java =================================================================== RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/ForkJoinPool.java,v retrieving revision 1.330 diff -u -r1.330 ForkJoinPool.java --- ForkJoinPool.java 3 Nov 2016 12:00:26 -0000 1.330 +++ ForkJoinPool.java 19 Jan 2017 19:27:38 -0000 @@ -615,14 +615,33 @@ public ForkJoinWorkerThread newThread(ForkJoinPool pool); } + static AccessControlContext accessControlContextWith( + RuntimePermission ... perms) { + Permissions permissions = new Permissions(); + for (RuntimePermission perm : perms) + permissions.add(perm); + return new AccessControlContext( + new ProtectionDomain[] { new ProtectionDomain(null, permissions) }); + } + /** * Default ForkJoinWorkerThreadFactory implementation; creates a * new ForkJoinWorkerThread. */ private static final class DefaultForkJoinWorkerThreadFactory implements ForkJoinWorkerThreadFactory { + private static final AccessControlContext ACC + = accessControlContextWith( + new RuntimePermission("getClassLoader"), + new RuntimePermission("setContextClassLoader")); + public final ForkJoinWorkerThread newThread(ForkJoinPool pool) { - return new ForkJoinWorkerThread(pool); + return AccessController.doPrivileged( + new PrivilegedAction<>() { + public ForkJoinWorkerThread run() { + return new ForkJoinWorkerThread( + pool, ClassLoader.getSystemClassLoader()); }}, + ACC); } } @@ -3215,18 +3234,12 @@ * An ACC to restrict permissions for the factory itself. * The constructed workers have no permissions set. */ - private static final AccessControlContext INNOCUOUS_ACC; - static { - Permissions innocuousPerms = new Permissions(); - innocuousPerms.add(modifyThreadPermission); - innocuousPerms.add(new RuntimePermission( - "enableContextClassLoaderOverride")); - innocuousPerms.add(new RuntimePermission( - "modifyThreadGroup")); - INNOCUOUS_ACC = new AccessControlContext(new ProtectionDomain[] { - new ProtectionDomain(null, innocuousPerms) - }); - } + private static final AccessControlContext INNOCUOUS_ACC + = accessControlContextWith( + modifyThreadPermission, + new RuntimePermission("enableContextClassLoaderOverride"), + new RuntimePermission("modifyThreadGroup"), + new RuntimePermission("setContextClassLoader")); public final ForkJoinWorkerThread newThread(ForkJoinPool pool) { return AccessController.doPrivileged(new PrivilegedAction<>() { Index: ForkJoinWorkerThread.java =================================================================== RCS file: /export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/ForkJoinWorkerThread.java,v retrieving revision 1.72 diff -u -r1.72 ForkJoinWorkerThread.java --- ForkJoinWorkerThread.java 3 Jun 2016 17:18:52 -0000 1.72 +++ ForkJoinWorkerThread.java 19 Jan 2017 19:27:38 -0000 @@ -7,6 +7,8 @@ package java.util.concurrent; import java.security.AccessControlContext; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.security.ProtectionDomain; /** @@ -59,11 +61,24 @@ } /** + * Version for use by the default pool. Supports setting the + * context class loader. This is a separate constructor to avoid + * affecting the protected constructor. + */ + ForkJoinWorkerThread(ForkJoinPool pool, ClassLoader ccl) { + super("aForkJoinWorkerThread"); + super.setContextClassLoader(ccl); + this.pool = pool; + this.workQueue = pool.registerWorker(this); + } + + /** * Version for InnocuousForkJoinWorkerThread. */ ForkJoinWorkerThread(ForkJoinPool pool, ThreadGroup threadGroup, AccessControlContext acc) { super(threadGroup, null, "aForkJoinWorkerThread"); + super.setContextClassLoader(null); // avoids classloader leak ThreadLocalRandom.setInheritedAccessControlContext(this, acc); ThreadLocalRandom.eraseThreadLocals(this); // clear before registering this.pool = pool; @@ -150,8 +165,8 @@ /** * A worker thread that has no permissions, is not a member of any - * user-defined ThreadGroup, and erases all ThreadLocals after - * running each top-level task. + * user-defined ThreadGroup, has no internal context class loader, + * and erases all ThreadLocals after running each top-level task. */ static final class InnocuousForkJoinWorkerThread extends ForkJoinWorkerThread { /** The ThreadGroup for all InnocuousForkJoinWorkerThreads */
19-01-2017
A permission check approach reasonable. I would be inclined to push a doPriv block in the special package protected constructor for the default ForkJoinWorkerThread, that will tighten the scope and there is no need to declare RuntimePermission instances. Regarding jdk.internal.misc.InnocuousThread, it may be possible to replace Unsafe usages with VarHandles and MethodHandles.privateLookupIn. I think it comes down to whether this class can be used early when the VM is booting. There is certainly a lot of similarity with InnocuousForkJoinWorkerThread (e.g. with ThreadGroups etc). I suggest we try and clean that up post 9.
19-01-2017
Here are the initial set of changes I think should address this issue. diff -r cb19d883084d src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java --- a/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java Wed Jan 18 11:18:13 2017 -0800 +++ b/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java Thu Jan 19 17:34:11 2017 +0000 @@ -651,7 +651,11 @@ private static final class DefaultForkJoinWorkerThreadFactory implements ForkJoinWorkerThreadFactory { public final ForkJoinWorkerThread newThread(ForkJoinPool pool) { - return new ForkJoinWorkerThread(pool); + PrivilegedAction<ForkJoinWorkerThread> pa = () -> + new ForkJoinWorkerThread(pool, ClassLoader.getSystemClassLoader()); + return AccessController.doPrivileged(pa, null, + new RuntimePermission("getClassLoader"), + new RuntimePermission("setContextClassLoader")); } } @@ -3252,6 +3256,8 @@ "enableContextClassLoaderOverride")); innocuousPerms.add(new RuntimePermission( "modifyThreadGroup")); + innocuousPerms.add(new RuntimePermission( + "setContextClassLoader")); INNOCUOUS_ACC = new AccessControlContext(new ProtectionDomain[] { new ProtectionDomain(null, innocuousPerms) }); diff -r cb19d883084d src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java --- a/src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java Wed Jan 18 11:18:13 2017 -0800 +++ b/src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java Thu Jan 19 17:34:11 2017 +0000 @@ -36,6 +36,8 @@ package java.util.concurrent; import java.security.AccessControlContext; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.security.ProtectionDomain; /** @@ -88,11 +90,23 @@ } /** + * Version for use by the default pool. Supports setting the context + * class loader. + */ + ForkJoinWorkerThread(ForkJoinPool pool, ClassLoader ccl) { // << a separate constructor to avoid affecting specified the protected constructor + super("aForkJoinWorkerThread"); + super.setContextClassLoader(ccl); + this.pool = pool; + this.workQueue = pool.registerWorker(this); + } + + /** * Version for InnocuousForkJoinWorkerThread. */ ForkJoinWorkerThread(ForkJoinPool pool, ThreadGroup threadGroup, AccessControlContext acc) { super(threadGroup, null, "aForkJoinWorkerThread"); + super.setContextClassLoader(null); ThreadLocalRandom.setInheritedAccessControlContext(this, acc); ThreadLocalRandom.eraseThreadLocals(this); // clear before registering this.pool = pool;
19-01-2017
If I'm right that "all we need to do" is to null out Thread's contextClassLoader field, we have a choice between super.setContextClassLoader(null), using Unsafe, or using a private VarHandle. Paul Sandoz should know best which approach is best.
19-01-2017
I see that jdk/internal/misc/InnocuousThread.java uses Unsafe to set Thread's contextClassLoader, which seems wrong. ("No one should need to resort to Unsafe anymore") But in principle there could be some sharing between the InnocuousThread implementations, and forkjoin could also set the field to null. The obvious way is super.setContextClassLoader(null), perhaps in a doPrivileged block. Would that work?
19-01-2017
It is already the case that InnocuousForkJoinWorkerThread does not allow TCCL to be used or set. The TCCL stored in the Thread object that is the source of the leak appears to be inaccessible to users. Can we just set it to null somehow? That should be compatible. @Override // to always report system loader public ClassLoader getContextClassLoader() { return ClassLoader.getSystemClassLoader(); } @Override // paranoically public void setContextClassLoader(ClassLoader cl) { throw new SecurityException("setContextClassLoader"); }
19-01-2017
David, I think we are in agreement. This can be changed in a major release, like 9, along with a release note.
19-01-2017
I think we should fix this in 9. It is highly fragile for user supplied tasks to depend on the TCCL within the common pool, since they have no insight into when the actual threads will be created, and by whom. There have been similar issues resolved in other areas, e.g. JDK-8157570, JDK-8156824, JDK-8156841, albeit these where internal threads that do not execute non-system code.
19-01-2017
If we mess with the context loader we may break apps that need to keep it set the way it currently is but don't set it explicitly. If we don't change it away from a short-lived loader then we may force that loader to remain reachable for the lifetime of the VM. Maybe we need to change the policy in 9 or 10, but I can't see any fix for this in 8u due to compatibility constraints
18-01-2017
It may not be reasonable for user supplied code to depend on the TCCL but they may do so anyway. But a change in 9 with a release note perhaps, should be okay.
18-01-2017
I'm slightly afraid to touch this myself. Can we get consulting help from those who helped previously with InnocuousForkJoinWorkerThreadFactory ?
13-01-2017
Issue is reproducible. To reproduce the issue, compile the attached java files and run the class ForkJoinThreadLeak Following are the results on various JDK versions: JDK 9ea + 151 - Fail JDK 8u121 b13 - Fail JDK 8u112 b15 - Fail JDK 7u131 b12 - Fail
12-01-2017