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.

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

Using own executor service #61

Merged
merged 6 commits into from
Jun 17, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
4 changes: 2 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@
</scm>

<properties>
<revision>2.4.2</revision>
<revision>2.5.0</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.60.3</jenkins.version>
<jenkins.version>2.164.3</jenkins.version>
Copy link
Member

@dwnusbaum dwnusbaum Jul 10, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

<java.level>8</java.level>
<no-test-jar>false</no-test-jar>
</properties>
Expand Down
23 changes: 18 additions & 5 deletions src/main/java/jenkins/scm/api/SCMEvent.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,17 @@
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.ClassLoaderSanityThreadFactory;
Copy link
Member

Choose a reason for hiding this comment

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

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

import hudson.util.DaemonThreadFactory;
import hudson.util.NamingThreadFactory;
import jenkins.security.ImpersonatingScheduledExecutorService;
import org.acegisecurity.context.SecurityContext;
import org.acegisecurity.context.SecurityContextHolder;
import org.apache.commons.lang.StringUtils;
Expand Down Expand Up @@ -126,6 +131,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 +208,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 ImpersonatingScheduledExecutorService(new ScheduledThreadPoolExecutor(10, new NamingThreadFactory(new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()), "SCMEvent")), ACL.SYSTEM);
}
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