Using own executor service #61

ingwarsw · 2019-01-07T09:08:23Z

In our company we have Jenkins with ~ 1000 repos in GitHub.
Most of them have a lot of tags (> 1k is standard, moved from old SCMs, but we cannot remove them so far)
Because of that processing events can take a loot of time, and Timer pool is really often all being used for event, which turns our jenkins into vegetable.

ingwarsw · 2019-03-22T09:10:58Z

Any thoughts about this?
It would be nice to include it someday, do I need to add some extra data?

bitwiseman · 2019-06-13T18:59:11Z

@ingwarsw
I'm not vary familiar with this area, but what you did here lines up with the "future-proofing" comment. 😄
It looks reasonable to me, but we should get another review before merging.

jglick · 2019-06-13T19:36:54Z

src/main/java/jenkins/scm/api/SCMEvent.java

+        if (executorService == null) {
+            // corePoolSize is set to 10, but will only be created if needed.
+            // ScheduledThreadPoolExecutor "acts as a fixed-sized pool using corePoolSize threads"
+            executorService =  new ScheduledThreadPoolExecutor(10, new NamingThreadFactory(new DaemonThreadFactory(), "SCMEvent"));


Note that Timer is also wrapped in utilities to

impersonate ACL.SYSTEM

handle errors, if we are not using SafeTimerTask

maintain the Thread.contextClassLoader

Whether any of those issues are important for this usage, I have no idea offhand. But perhaps Timer should get a utility factory

public static ScheduledExecutorService newService(String name, int corePoolSize) { return new ImpersonatingScheduledExecutorService(new ErrorLoggingScheduledThreadPoolExecutor(corePoolSize, new NamingThreadFactory(new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()), name)), ACL.SYSTEM); }

for easy reuse.

This one does everything except logging errors if we are not using SafeTimerTask

I can use exactly the same as in timer... but I choose to use that one for some purpose..
It was so long ago I dont even remember why..
But I remember it was not accident..

And Timer is in core so I cannot change it from here..

This one does everything except logging errors

It does not do the impersonation or context class loader maintenance.

Timer is in core so I cannot change it from here

Of course, but we can have a core PR, and via JEP-305 it is even straightforward to have an on-hold PR in the plugin which verifies its usage.

I see that there was change in Timer.. ok.. so it does more now..

So it would be great to move it somewhere in one place..

And maybe it would be easier and better to just allow changing size of timer pool via variable or something?
Here https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/util/Timer.java#L47

they sometimes takes hours to complete

A thread dump would be the background we need to analyze why.

I can get thread dump.. just need to wait for busy day..
But all waits here https://github.com/kohsuke/github-api/blob/master/src/main/java/org/kohsuke/github/RateLimitHandler.java#L39
We are just got rate limit badly..

And while we dont have problems with waiting for GH..
We have problem that nothing else (that relies on Timer) on jenkins is working..

Need the thread dump to see what caller is to blame.

Webhook processing is going into this pool..
And there is issue with GH processing itself (sometimes rate limiting is not resetting)..
But not that easy to grasp..
And would be best to fix somewhere here..
https://github.com/kohsuke/github-api/blob/master/src/main/java/org/kohsuke/github/GitHub.java#L316
Once for good :)

stephenc

Ahhh it’s the rate limit back-off in the event handling. Yes event handling needs it’s own pool. I’ll take a look at this PR later, but in principle the request is sound.

ingwarsw · 2019-06-13T21:17:21Z

Guys I can update it to looks exactly like in Timer but I will have to update jenkins core version 2.60.3 -> 2.107.1 to have all required classes.

stephenc · 2019-06-13T22:01:38Z

Go for the core version bump. Seems entirely reasonable to bump to that LTS at this time. Because of the core version bump, also bump the plugin’s minor version and reset patch to 0

ingwarsw · 2019-06-14T08:49:48Z

@stephenc Done..

stephenc

Minor formatting
We should be a good citizen in the servlet spec (even if most users just kill the servlet container, it will affect downstream plugin testing as the thread pool will retain resources and more critically it will prevent the loaded classes from being unloaded and represent a memory leak when running the tests)

stephenc · 2019-06-17T09:21:21Z

src/main/java/jenkins/scm/api/SCMEvent.java

 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import javax.servlet.http.HttpServletRequest;
-import jenkins.util.Timer;
+
+import hudson.util.ClassLoaderSanityThreadFactory;


all imports in a-z order, no blank lines please

stephenc · 2019-06-17T09:24:35Z

src/main/java/jenkins/scm/api/SCMEvent.java

+            // ScheduledThreadPoolExecutor "acts as a fixed-sized pool using corePoolSize threads"
+            executorService = new ImpersonatingScheduledExecutorService(new ScheduledThreadPoolExecutor(10, new NamingThreadFactory(new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()), "SCMEvent")), ACL.SYSTEM);
+        }
+        return executorService;
    }



we'll need a @Terminator annotated static method that shuts down this pool to play nicely with servlet containers that get reused, e.g. if anyone is in a downstream plugin and has lots of event unit tests, they may end up with lots of these thread pools. Better to expressly shut them down with the Jenkins lifecycle.

See https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/init/Terminator.java

stephenc

Clicked wrong button... 2 changes per previous comment

ingwarsw · 2019-06-17T09:34:29Z

@stephenc Terminator in that form?

gmshake · 2019-06-18T10:11:38Z

src/main/java/jenkins/scm/api/SCMEvent.java

+     * Shutdown the timer and throw it away.
+     */
+    @Terminator
+    public synchronized void closeExecutorService() {


Need proper synchronizing here. Accidentally miss static modifiers ?

See previous

protected static synchronized ScheduledExecutorService executorService() { ... }

@ingwarsw CC @stephenc

Actually I didnt think about it..

And I removed static because Im not sure if @Terminator can work on static method..
But if works we should add that static back to be on safe side..

I see even @stephenc mentioned it should be static..
I will create fast fixing PR..

dwnusbaum · 2019-07-10T14:48:26Z

pom.xml

    <changelist>-SNAPSHOT</changelist>
-    <jenkins.version>2.60.3</jenkins.version>
+    <jenkins.version>2.164.3</jenkins.version>


FWIW usually preferable to update to the oldest LTS line that has whatever core API you need. Especially in this case, because the plugin is an API intended to be used by other plugins, requiring the second-newest LTS release means that any plugin that wants to pick up the latest version of this plugin (for example jenkinsci/workflow-cps-global-lib-plugin#68) also requires that release, which is a bit prohibitive.

In this case 2.107.3 was the minimum required baseline as far as I know.

ingwarsw added 2 commits January 7, 2019 10:00

Using own executor service

f5b5887

Trigger notification

aa36937

bitwiseman approved these changes Jun 13, 2019

View reviewed changes

bitwiseman closed this Jun 13, 2019

bitwiseman reopened this Jun 13, 2019

bitwiseman requested a review from jglick June 13, 2019 19:08

bitwiseman closed this Jun 13, 2019

bitwiseman reopened this Jun 13, 2019

jglick approved these changes Jun 13, 2019

View reviewed changes

jglick requested a review from stephenc June 13, 2019 19:37

Merge remote-tracking branch 'jenkins/master' into own_executors_service

7c7b6e1

stephenc reviewed Jun 13, 2019

View reviewed changes

ingwarsw added 2 commits June 14, 2019 10:41

Use version of scheduler as in current version of Timer

066f398

Update core version to current LTS + minor version bump

713c127

stephenc reviewed Jun 17, 2019

View reviewed changes

stephenc requested changes Jun 17, 2019

View reviewed changes

Fixes after review..

38c2c5a

stephenc approved these changes Jun 17, 2019

View reviewed changes

stephenc merged commit df46ae7 into jenkinsci:master Jun 17, 2019

gmshake reviewed Jun 20, 2019

View reviewed changes

jglick mentioned this pull request Jul 10, 2019

[JENKINS-43802] No context available for credentials from SCMSource.retrieve(String, TaskListener) jenkinsci/workflow-cps-global-lib-plugin#68

Merged

dwnusbaum reviewed Jul 10, 2019

View reviewed changes

jglick mentioned this pull request Jul 11, 2019

Push minimum jenkins version back to 2.107.3 #73

Merged

timja mentioned this pull request Mar 25, 2022

JENKINS-68116 Make event thread pool size customize-able #124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using own executor service #61

Using own executor service #61

ingwarsw commented Jan 7, 2019

ingwarsw commented Mar 22, 2019

bitwiseman commented Jun 13, 2019

jglick Jun 13, 2019

ingwarsw Jun 13, 2019

ingwarsw Jun 13, 2019

jglick Jun 13, 2019

ingwarsw Jun 13, 2019

jglick Jun 13, 2019

ingwarsw Jun 13, 2019

ingwarsw Jun 13, 2019

jglick Jun 13, 2019 •

edited

Loading

ingwarsw Jun 13, 2019

stephenc left a comment

ingwarsw commented Jun 13, 2019

stephenc commented Jun 13, 2019

ingwarsw commented Jun 14, 2019

stephenc left a comment

stephenc Jun 17, 2019

stephenc Jun 17, 2019

stephenc left a comment

ingwarsw commented Jun 17, 2019 •

edited

Loading

gmshake Jun 18, 2019

ingwarsw Jun 20, 2019

ingwarsw Jun 20, 2019

ingwarsw Jun 20, 2019

dwnusbaum Jul 10, 2019 •

edited

Loading

jglick Jul 10, 2019

Using own executor service #61

Using own executor service #61

Conversation

ingwarsw commented Jan 7, 2019

ingwarsw commented Mar 22, 2019

bitwiseman commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc left a comment

Choose a reason for hiding this comment

ingwarsw commented Jun 13, 2019

stephenc commented Jun 13, 2019

ingwarsw commented Jun 14, 2019

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc left a comment

Choose a reason for hiding this comment

ingwarsw commented Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick Jun 13, 2019 •

edited

Loading

ingwarsw commented Jun 17, 2019 •

edited

Loading

dwnusbaum Jul 10, 2019 •

edited

Loading