From 2a6fc653ee7b13adde18515b21f7e6dc1200fa8a Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Sat, 3 Feb 2018 07:22:53 -0500 Subject: [PATCH] Fix [JENKINS-49206] by ensuring Timer threads get standard classloader (#3272) * Fix [JENKINS-49206] by ensuring Timer threads get standard classloader * Refactor to better fit in with existing ThreadFactor utils * Fix javadocs --- .../util/ClassLoaderSanityThreadFactory.java | 26 +++++++++ core/src/main/java/jenkins/util/Timer.java | 6 ++- .../src/test/java/jenkins/util/TimerTest.java | 54 +++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 core/src/main/java/hudson/util/ClassLoaderSanityThreadFactory.java diff --git a/core/src/main/java/hudson/util/ClassLoaderSanityThreadFactory.java b/core/src/main/java/hudson/util/ClassLoaderSanityThreadFactory.java new file mode 100644 index 0000000000..041c2061df --- /dev/null +++ b/core/src/main/java/hudson/util/ClassLoaderSanityThreadFactory.java @@ -0,0 +1,26 @@ +package hudson.util; + +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; + +/** + * Explicitly sets the {@link Thread#contextClassLoader} for threads it creates to its own classloader. + * This avoids issues where threads are lazily created (ex by invoking {@link java.util.concurrent.ScheduledExecutorService#schedule(Runnable, long, TimeUnit)}) + * in a context where they would receive a customized {@link Thread#contextClassLoader} that was never meant to be used. + * + * Commonly this is a problem for Groovy use, where this may result in memory leaks. + * @see JENKINS-49206 + */ +public class ClassLoaderSanityThreadFactory implements ThreadFactory { + private final ThreadFactory delegate; + + public ClassLoaderSanityThreadFactory(ThreadFactory delegate) { + this.delegate = delegate; + } + + @Override public Thread newThread(Runnable r) { + Thread t = delegate.newThread(r); + t.setContextClassLoader(ClassLoaderSanityThreadFactory.class.getClassLoader()); + return t; + } +} diff --git a/core/src/main/java/jenkins/util/Timer.java b/core/src/main/java/jenkins/util/Timer.java index b452efa062..65acffe4f7 100644 --- a/core/src/main/java/jenkins/util/Timer.java +++ b/core/src/main/java/jenkins/util/Timer.java @@ -1,6 +1,7 @@ package jenkins.util; import hudson.security.ACL; +import hudson.util.ClassLoaderSanityThreadFactory; import hudson.util.DaemonThreadFactory; import hudson.util.NamingThreadFactory; import javax.annotation.Nonnull; @@ -29,7 +30,8 @@ public class Timer { * The scheduled executor thread pool. This is initialized lazily since it may be created/shutdown many times * when running the test suite. */ - private static ScheduledExecutorService executorService; + static ScheduledExecutorService executorService; + /** * Returns the scheduled executor service used by all timed tasks in Jenkins. @@ -42,7 +44,7 @@ public class Timer { // corePoolSize is set to 10, but will only be created if needed. // ScheduledThreadPoolExecutor "acts as a fixed-sized pool using corePoolSize threads" // TODO consider also wrapping in ContextResettingExecutorService - executorService = new ImpersonatingScheduledExecutorService(new ErrorLoggingScheduledThreadPoolExecutor(10, new NamingThreadFactory(new DaemonThreadFactory(), "jenkins.util.Timer")), ACL.SYSTEM); + executorService = new ImpersonatingScheduledExecutorService(new ErrorLoggingScheduledThreadPoolExecutor(10, new NamingThreadFactory(new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()), "jenkins.util.Timer")), ACL.SYSTEM); } return executorService; } diff --git a/core/src/test/java/jenkins/util/TimerTest.java b/core/src/test/java/jenkins/util/TimerTest.java index cbe40de66d..507c117739 100644 --- a/core/src/test/java/jenkins/util/TimerTest.java +++ b/core/src/test/java/jenkins/util/TimerTest.java @@ -1,12 +1,18 @@ package jenkins.util; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; +import groovy.lang.GroovyClassLoader; import hudson.triggers.SafeTimerTask; +import org.junit.Assert; import org.junit.Test; import org.jvnet.hudson.test.Issue; +import java.net.URLClassLoader; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; public class TimerTest { @@ -44,4 +50,52 @@ public class TimerTest { } } + + /** + * Launch two tasks which can only complete + * by running doRun() concurrently. + */ + @Test + @Issue("JENKINS-49206") + public void timerBogusClassloader() throws Exception { + final int threadCount = 10; // Twice Timer pool size to ensure we end up creating at least one new thread + final CountDownLatch startLatch = new CountDownLatch(threadCount); + + final ClassLoader[] contextClassloaders = new ClassLoader[threadCount]; + ScheduledFuture[] futures = new ScheduledFuture[threadCount]; + final ClassLoader bogusClassloader = new GroovyClassLoader(); + + Runnable timerTest = new Runnable() { + @Override + public void run() { + ClassLoader cl = Thread.currentThread().getContextClassLoader(); + Thread.currentThread().setContextClassLoader(bogusClassloader); + ScheduledExecutorService exec = Timer.get(); + for (int i=0; i