Skip to content

Commit

Permalink
[JENKINS-72796] stable context classloader for Computer.threadPoolFor…
Browse files Browse the repository at this point in the history
…Remoting

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.
  • Loading branch information
jtnord committed Mar 4, 2024
1 parent 19f0140 commit bf53852
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 1 deletion.
5 changes: 4 additions & 1 deletion core/src/main/java/hudson/model/Computer.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));

//
//
Expand Down
40 changes: 40 additions & 0 deletions core/src/test/java/hudson/model/ComputerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}

0 comments on commit bf53852

Please sign in to comment.