From db0dc64d663e46494c1588f067f61c0fc6a2b241 Mon Sep 17 00:00:00 2001
From: James Nord <jtnord@users.noreply.github.com>
Date: Wed, 6 Mar 2024 14:19:31 +0000
Subject: [PATCH] [JENKINS-72796] stable context classloader for
 Computer.threadPoolForRemoting (#9012)

* [JENKINS-72796] stable context classloader for Computer.threadPoolForRemoting

Whilst the threadpool used reset the context classloader at the end of
any task, it did not ensure that the initial c;lassloader used was
anything sepcific, rather it would use whatever the calling threads
contextClassLoader was.

This is now fixed as we use the Jenkins WebApp classloader (same as
the Timer) which is used by (A)PeriodicTasks.

Whilst we should really not have a context classloader (aka null) and
this should be set where needed by code, almost everywhere in Jenkins
the context classloader is already the webapp classloader, and so
setting this to be different depending on how things where called would
seemingly be a little scary.  Arguably this and other context
classloaders should be all set to null and any code that wants different
should be changed, but this is a larger piece of work that would have
potential impact on an unknown number of plugins in the ecosystem, so
this fix uses what was set > 90% of the time.

* Update core/src/test/java/hudson/model/ComputerTest.java

---------

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
(cherry picked from commit 89195cc248eb973dae4212d613914d616805bc1d)
---
 core/src/main/java/hudson/model/Computer.java |  5 ++-
 .../test/java/hudson/model/ComputerTest.java  | 40 +++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java
index 1759397d0b6d7..ad09c189edf78 100644
--- a/core/src/main/java/hudson/model/Computer.java
+++ b/core/src/main/java/hudson/model/Computer.java
@@ -63,6 +63,7 @@
 import hudson.slaves.RetentionStrategy;
 import hudson.slaves.WorkspaceList;
 import hudson.triggers.SafeTimerTask;
+import hudson.util.ClassLoaderSanityThreadFactory;
 import hudson.util.DaemonThreadFactory;
 import hudson.util.EditDistance;
 import hudson.util.ExceptionCatchingThreadFactory;
@@ -1381,7 +1382,9 @@ public String call() throws IOException {
                 Executors.newCachedThreadPool(
                     new ExceptionCatchingThreadFactory(
                         new NamingThreadFactory(
-                            new DaemonThreadFactory(), "Computer.threadPoolForRemoting")))), ACL.SYSTEM2));
+                            new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()),
+                            "Computer.threadPoolForRemoting")))),
+            ACL.SYSTEM2));
 
 //
 //
diff --git a/core/src/test/java/hudson/model/ComputerTest.java b/core/src/test/java/hudson/model/ComputerTest.java
index d7c27880c1af8..bd5ddda4ab8c5 100644
--- a/core/src/test/java/hudson/model/ComputerTest.java
+++ b/core/src/test/java/hudson/model/ComputerTest.java
@@ -8,9 +8,11 @@
 import hudson.FilePath;
 import hudson.security.ACL;
 import java.io.File;
+import java.util.ArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import jenkins.model.Jenkins;
+import jenkins.util.SetContextClassLoader;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
 import org.springframework.security.core.Authentication;
@@ -45,4 +47,42 @@ public void testThreadPoolForRemotingActsAsSystemUser() throws InterruptedExcept
         Future<Authentication> job = Computer.threadPoolForRemoting.submit(Jenkins::getAuthentication2);
         assertThat(job.get(), is(ACL.SYSTEM2));
     }
+
+    @Issue("JENKINS-72796")
+    @Test
+    public void testThreadPoolForRemotingContextClassLoaderIsSet() throws Exception {
+        // as the threadpool is cached, any other tests here pollute this test so we need enough threads to
+        // avoid any cached.
+        final int numThreads = 5;
+
+        // simulate the first call to Computer.threadPoolForRemoting with a non default classloader
+        try (var ignored = new SetContextClassLoader(new ClassLoader() {})) {
+            obtainAndCheckThreadsContextClassloaderAreCorrect(numThreads);
+        }
+        // now repeat this as the checking that the pollution of the context classloader is handled
+        obtainAndCheckThreadsContextClassloaderAreCorrect(numThreads);
+    }
+
+    private static void obtainAndCheckThreadsContextClassloaderAreCorrect(int numThreads) throws Exception {
+        ArrayList<Future<ClassLoader>> classloaderFuturesList = new ArrayList<>();
+        // block all calls to getContextClassloader() so we create more threads.
+        synchronized (WaitAndGetContextClassLoader.class) {
+            for (int i = 0; i < numThreads; i++) {
+                classloaderFuturesList.add(Computer.threadPoolForRemoting.submit(WaitAndGetContextClassLoader::getContextClassloader));
+            }
+        }
+        for (Future<ClassLoader> fc : classloaderFuturesList) {
+            assertThat(fc.get(), is(Jenkins.class.getClassLoader()));
+        }
+    }
+
+    private static class WaitAndGetContextClassLoader {
+
+        public static synchronized ClassLoader getContextClassloader() throws InterruptedException {
+            ClassLoader ccl = Thread.currentThread().getContextClassLoader();
+            // intentionally pollute the Threads context classloader
+            Thread.currentThread().setContextClassLoader(new ClassLoader() {});
+            return ccl;
+        }
+    }
 }