timja / jenkins-gh-issues-poc-06-18

0 stars 0 forks source link

[JENKINS-67159] Incorrect creation of ThreadPoolExecutor in Jenkins core #1735

Open timja opened 2 years ago

timja commented 2 years ago

Jenkins has 3 Executor Services that use cached thread pools that are static.

This causes issues as the when created the executor service will store the ThreadGroup of the calling thread (the thread that caused the class to be loaded).
This calling Thread could be anything - and even a member of a deamon ThreadGroup.

If the ThreadGroup is deamon then it is terminated when there are no longer any members of it - which then leads to failure to submit new tasks to the service as a new Thread will not be able to be created.

Additionally as the Executor Services are static - they are not shutdown when Jenkins terminates and as such we can leak threads (if restarting) or not terminate running tasks correctly.

Observed so far only in tests, but code inspection reveals that this could be triggered in a production instance.

Seems like a bug in Proc, and perhaps other similar code (UpdateCenter, FilePath, Computer, more?). Timer takes care to actually shut down properly; does not look like any of the others do.

[Mon Aug 23 16:25:48 UTC 2021] Finished branch indexing. Indexing took 49 ms
FATAL: Failed to recompute children of test1
java.lang.IllegalThreadStateException
    at java.lang.ThreadGroup.addUnstarted(ThreadGroup.java:867)
    at java.lang.Thread.init(Thread.java:405)
    at java.lang.Thread.init(Thread.java:349)
    at java.lang.Thread.(Thread.java:678)
    at java.util.concurrent.Executors$DefaultThreadFactory.newThread(Executors.java:613)
    at hudson.util.DaemonThreadFactory.newThread(DaemonThreadFactory.java:47)
    at hudson.util.ClassLoaderSanityThreadFactory.newThread(ClassLoaderSanityThreadFactory.java:23)
    at hudson.util.NamingThreadFactory.newThread(NamingThreadFactory.java:52)
    at hudson.util.ExceptionCatchingThreadFactory.newThread(ExceptionCatchingThreadFactory.java:51)
    at java.util.concurrent.ThreadPoolExecutor$Worker.(ThreadPoolExecutor.java:619)
    at java.util.concurrent.ThreadPoolExecutor.addWorker(ThreadPoolExecutor.java:932)
    at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1378)
    at java.util.concurrent.AbstractExecutorService.submit(AbstractExecutorService.java:112)
    at hudson.Proc.joinWithTimeout(Proc.java:159)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2670)


Originally reported by teilo, imported from: Incorrect creation of ThreadPoolExecutor in Jenkins core
  • status: Open
  • priority: Minor
  • resolution: Unresolved
  • imported: 2022/01/10
timja commented 2 years ago

jglick:

as the Executor Services are static - they are not shutdown when Jenkins terminates

https://github.com/jenkinsci/jenkins/blob/1c271f5e167656d3abbe0fb20837f60459187043/core/src/main/java/jenkins/model/Jenkins.java#L3778-L3781

Generally speaking, creation of new thread pools should be avoided—reuse one of the standard ones (subject to its constraints).

the executor service will store the ThreadGroup of the calling thread

This sounds really familiar. I feel like this was solved at one point in Jenkins. Or maybe NetBeans? IIRC you need to ensure that you walk up to the root group. https://github.com/apache/netbeans/blob/0ffa067ea30747ab73f077b8495a297f5520a91f/platform/core.execution/src/org/netbeans/core/execution/ExecutionEngine.java#L230-L242 https://github.com/apache/netbeans/blob/0ffa067ea30747ab73f077b8495a297f5520a91f/platform/openide.util/src/org/openide/util/RequestProcessor.java#L1076-L1082 etc.