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
jenkinsci · Feb 3, 2018 · 2a6fc65 · 2a6fc65
1 parent 6cefcf1
commit 2a6fc65
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 2 deletions.
diff --git a/core/src/main/java/hudson/util/ClassLoaderSanityThreadFactory.java 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 <a href="https://issues.jenkins-ci.org/browse/JENKINS-49206">JENKINS-49206</a>
+ */
+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
@@ -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 static synchronized ScheduledExecutorService get() {
             // 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
@@ -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 @@ protected void doRun() throws Exception {
         }
 
     }
+
+    /**
+     * 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<threadCount; i++) {
+                    final int j = i;
+                    futures[j] = exec.schedule(new Runnable() {
+                        @Override
+                        public void run() {
+                            try {
+                                startLatch.countDown();
+                                contextClassloaders[j] = Thread.currentThread().getContextClassLoader();
+                            } catch (Exception ex) {
+                                throw  new RuntimeException(ex);
+                            }
+                        }
+                    }, 0, TimeUnit.SECONDS);
+                }
+                Thread.currentThread().setContextClassLoader(cl);
+            }
+        };
+
+        Thread t = new Thread(timerTest);
+        t.run();
+        t.join(1000L);
+
+        for (int i=0; i<threadCount; i++) {
+            futures[i].get();
+            Assert.assertEquals(Timer.class.getClassLoader(), contextClassloaders[i]);
+        }
+    }
 }