From da4ed53fb8f38f10df956175e7feda76f64f766e Mon Sep 17 00:00:00 2001 From: Ondrej Kmoch Date: Thu, 26 Jul 2018 16:38:47 +0200 Subject: [PATCH 1/2] Moving check for new Pull Requests into separate thread pool to avoid blocking cron thread (JENKINS-41336) --- .../StashBuildTrigger.java | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java b/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java index a91bf676..68654309 100644 --- a/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java +++ b/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java @@ -25,7 +25,10 @@ import hudson.security.ACL; import hudson.triggers.Trigger; import hudson.triggers.TriggerDescriptor; +import hudson.util.DaemonThreadFactory; +import hudson.util.ExceptionCatchingThreadFactory; import hudson.util.ListBoxModel; +import hudson.util.NamingThreadFactory; import jenkins.model.Jenkins; import jenkins.model.ParameterizedJobMixIn; import net.sf.json.JSONObject; @@ -41,6 +44,9 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; import java.util.logging.Logger; @@ -71,6 +77,7 @@ public class StashBuildTrigger extends Trigger> { private final boolean cancelOutdatedJobsEnabled; transient private StashPullRequestsBuilder stashPullRequestsBuilder; + transient private AtomicBoolean checkAlreadyQueued; @Extension public static final StashBuildTriggerDescriptor descriptor = new StashBuildTriggerDescriptor(); @@ -202,6 +209,7 @@ public void start(Job job, boolean newInstance) { this.stashPullRequestsBuilder.setJob(job); this.stashPullRequestsBuilder.setTrigger(this); this.stashPullRequestsBuilder.setupBuilder(); + this.checkAlreadyQueued = new AtomicBoolean(false); } catch(IllegalStateException e) { logger.log(Level.SEVERE, "Can't start trigger", e); return; @@ -213,7 +221,7 @@ public static StashBuildTrigger getTrigger(Job job) { if (!(job instanceof ParameterizedJobMixIn.ParameterizedJob)) { return null; } - + ParameterizedJobMixIn.ParameterizedJob pjob = (ParameterizedJobMixIn.ParameterizedJob) job; Trigger trigger = pjob.getTriggers().get(descriptor); @@ -248,13 +256,13 @@ public QueueTaskFuture startJob(StashCause cause) { cancelPreviousJobsInQueueThatMatch(cause); abortRunningJobsThatMatch(cause); } - + return new ParameterizedJobMixIn() { @Override protected Job asJob() { return StashBuildTrigger.this.job; } - }.scheduleBuild2(0, new ParametersAction(values), new CauseAction(cause)); + }.scheduleBuild2(0, new ParametersAction(values), new CauseAction(cause)); } private void cancelPreviousJobsInQueueThatMatch(@Nonnull StashCause stashCause) { @@ -309,11 +317,22 @@ private List getDefaultParameters() { @Override public void run() { - if(!this.getBuilder().getJob().isBuildable()) { + if (!this.getBuilder().getJob().isBuildable()) { logger.info(format("Build Skip (%s).", getBuilder().getJob().getName())); - } else { + } else if(checkAlreadyQueued.compareAndSet(false, true)) { logger.info(format("Build started (%s).", getBuilder().getJob().getName())); - this.stashPullRequestsBuilder.run(); + descriptor.getExecutorService().execute(new Runnable() { + @Override + public void run() { + try { + stashPullRequestsBuilder.run(); + } finally { + checkAlreadyQueued.set(false); + } + } + }); + } else { + logger.info(format("Build check already queued - skipping (%s).", getBuilder().getJob().getName())); } this.getDescriptor().save(); } @@ -336,10 +355,22 @@ public boolean isOnlyBuildOnComment() { } public static final class StashBuildTriggerDescriptor extends TriggerDescriptor { + + private transient final ExecutorService executorService; + public StashBuildTriggerDescriptor() { + executorService = Executors.newFixedThreadPool( + 10, + new ExceptionCatchingThreadFactory( + new NamingThreadFactory(new DaemonThreadFactory(), "StashBuildTriggerDescriptor.executorService"))); load(); } + @Nonnull + public ExecutorService getExecutorService() { + return executorService; + } + @Override public boolean isApplicable(Item item) { return true; From 2a06aa472454445911d100a74bc5de6d3ffcf161 Mon Sep 17 00:00:00 2001 From: Ondrej Kmoch Date: Thu, 6 Dec 2018 10:37:11 +0100 Subject: [PATCH 2/2] Adding exception handling for checking for new Pull Requests to be logged --- .../stashpullrequestbuilder/StashBuildTrigger.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java b/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java index 68654309..a2a26d57 100644 --- a/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java +++ b/src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java @@ -326,6 +326,8 @@ public void run() { public void run() { try { stashPullRequestsBuilder.run(); + } catch (Exception e) { + logger.log(Level.WARNING, "Stash Pull Request Builder failed when checking for changes", e); } finally { checkAlreadyQueued.set(false); }