Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Using own executor service #61

Merged
merged 6 commits into from
Jun 17, 2019
Merged

Using own executor service #61

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f5b5887
Using own executor service
ingwarsw Jan 7, 2019
aa36937
Trigger notification
ingwarsw Jan 9, 2019
7c7b6e1
Merge remote-tracking branch 'jenkins/master' into own_executors_service
ingwarsw Jun 13, 2019
066f398
Use version of scheduler as in current version of Timer
ingwarsw Jun 14, 2019
713c127
Update core version to current LTS + minor version bump
ingwarsw Jun 14, 2019
38c2c5a
Fixes after review..
ingwarsw Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/main/java/jenkins/scm/api/SCMEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,15 @@
import hudson.security.ACL;
import java.util.Date;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledThreadPoolExecutor;
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.DaemonThreadFactory;
import hudson.util.NamingThreadFactory;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.apache.commons.lang.StringUtils;
Expand Down Expand Up @@ -126,6 +129,11 @@ public abstract class SCMEvent<P> {
@CheckForNull
private final String origin;

/**
* The scheduled executor thread pool. This is initialized lazily since it may be never needed.
*/
private static ScheduledExecutorService executorService;

/**
* Constructor to use when the timestamp is available from the external SCM.
*
Expand Down Expand Up @@ -198,10 +206,13 @@ protected SCMEvent(SCMEvent<P> copy) {
* @return a {@link ScheduledExecutorService}.
*/
@NonNull
protected static ScheduledExecutorService executorService() {
// Future-proofing, if we find out that events are drowning Timer then we may need to move them to their
// own dedicated ScheduledExecutorService thread pool
return Timer.get();
protected static synchronized ScheduledExecutorService executorService() {
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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they sometimes takes hours to complete

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jglick jglick Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

}
return executorService;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
Expand Down