From f719bd06a46b0c8f8249f5d4848db61947b920be Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 25 Jun 2013 11:15:32 -0700 Subject: [PATCH] [FIXED JENKINS-18407] Added Queue.schedule2 to allow the caller to retrieve the existing item in the queue. AbstractProject.doBuild() changed the behavior a bit to reply 201 if the item was already found in the queue (instead of a new one created.) --- .../hudson/matrix/MatrixConfiguration.java | 7 +- .../matrix/listeners/MatrixBuildListener.java | 2 +- .../java/hudson/model/AbstractProject.java | 14 +-- core/src/main/java/hudson/model/Queue.java | 44 +++++-- .../hudson/model/queue/ScheduleResult.java | 111 ++++++++++++++++++ .../hudson/model/AbstractProjectTest.groovy | 81 ++++++++++--- 6 files changed, 221 insertions(+), 38 deletions(-) create mode 100644 core/src/main/java/hudson/model/queue/ScheduleResult.java diff --git a/core/src/main/java/hudson/matrix/MatrixConfiguration.java b/core/src/main/java/hudson/matrix/MatrixConfiguration.java index b1c56409be..2b3694392e 100644 --- a/core/src/main/java/hudson/matrix/MatrixConfiguration.java +++ b/core/src/main/java/hudson/matrix/MatrixConfiguration.java @@ -393,17 +393,16 @@ public class MatrixConfiguration extends Project * @param parameters * Can be null. * @deprecated - * Use {@link #scheduleBuild(List, Cause)}. Since 1.480 + * Use {@link #scheduleBuild(List, Cause)}. Since 1.480 */ public boolean scheduleBuild(ParametersAction parameters, Cause c) { - return scheduleBuild(Collections.singletonList(parameters), c); } /** * Starts the build with the actions that are passed in. * * @param actions Can be null. - * @param cause Reason for starting the build + * @param c Reason for starting the build */ public boolean scheduleBuild(List actions, Cause c) { List allActions = new ArrayList(); @@ -414,7 +413,7 @@ public class MatrixConfiguration extends Project allActions.add(new ParentBuildAction()); allActions.add(new CauseAction(c)); - return Jenkins.getInstance().getQueue().schedule(this, getQuietPeriod(), allActions )!=null; + return Jenkins.getInstance().getQueue().schedule2(this, getQuietPeriod(), allActions ).isAccepted(); } /** diff --git a/core/src/main/java/hudson/matrix/listeners/MatrixBuildListener.java b/core/src/main/java/hudson/matrix/listeners/MatrixBuildListener.java index 4bb7076f23..53c37003a4 100644 --- a/core/src/main/java/hudson/matrix/listeners/MatrixBuildListener.java +++ b/core/src/main/java/hudson/matrix/listeners/MatrixBuildListener.java @@ -41,7 +41,7 @@ import java.util.List; *

* Plugins can implement this extension point to filter out the subset of matrix project to build. * Most typically, such a plugin would add a custom {@link Action} to a build when it goes to the queue - * ({@link Queue#schedule(Task, int, List)}, then access this from {@link MatrixBuild} to drive + * ({@link Queue#schedule2(Task, int, List)}, then access this from {@link MatrixBuild} to drive * the filtering logic. * *

diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java index e0877c0a94..f996d78cf4 100644 --- a/core/src/main/java/hudson/model/AbstractProject.java +++ b/core/src/main/java/hudson/model/AbstractProject.java @@ -48,8 +48,8 @@ import hudson.model.PermalinkProjectAction.Permalink; import hudson.model.Queue.Executable; import hudson.model.Queue.Task; import hudson.model.queue.QueueTaskFuture; +import hudson.model.queue.ScheduleResult; import hudson.model.queue.SubTask; -import hudson.model.Queue.WaitingItem; import hudson.model.RunMap.Constructor; import hudson.model.labels.LabelAtom; import hudson.model.labels.LabelExpression; @@ -918,9 +918,9 @@ public abstract class AbstractProject

,R extends A queueActions.add(new CauseAction(c)); } - WaitingItem i = Jenkins.getInstance().getQueue().schedule(this, quietPeriod, queueActions); - if(i!=null) - return (QueueTaskFuture)i.getFuture(); + ScheduleResult i = Jenkins.getInstance().getQueue().schedule2(this, quietPeriod, queueActions); + if(i.isAccepted()) + return (QueueTaskFuture)i.getItem().getFuture(); return null; } @@ -1812,9 +1812,9 @@ public abstract class AbstractProject

,R extends A if (!isBuildable()) throw HttpResponses.error(SC_INTERNAL_SERVER_ERROR,new IOException(getFullName()+" is not buildable")); - WaitingItem item = Jenkins.getInstance().getQueue().schedule(this, (int) delay.getTime(), getBuildCause(req)); - if (item!=null) { - rsp.sendRedirect(SC_CREATED,req.getContextPath()+'/'+item.getUrl()); + ScheduleResult r = Jenkins.getInstance().getQueue().schedule2(this, delay.getTime(), getBuildCause(req)); + if (r.isAccepted()) { + rsp.sendRedirect(SC_CREATED,req.getContextPath()+'/'+r.getItem().getUrl()); } else rsp.sendRedirect("."); } diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index ca25158f89..cc491b2199 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -46,6 +46,8 @@ import hudson.model.queue.AbstractQueueTask; import hudson.model.queue.Executables; import hudson.model.queue.QueueListener; import hudson.model.queue.QueueTaskFuture; +import hudson.model.queue.ScheduleResult; +import hudson.model.queue.ScheduleResult.Created; import hudson.model.queue.SubTask; import hudson.model.queue.FutureImpl; import hudson.model.queue.MappingWorksheet; @@ -473,6 +475,14 @@ public class Queue extends ResourceController implements Saveable { return schedule(p, quietPeriod)!=null; } + /** + * @deprecated as of 1.521 + * Use {@link #schedule2(Task, int, List)} + */ + public synchronized WaitingItem schedule(Task p, int quietPeriod, List actions) { + return schedule2(p, quietPeriod, actions).getCreateItem(); + } + /** * Schedules an execution of a task. * @@ -483,14 +493,18 @@ public class Queue extends ResourceController implements Saveable { * For the convenience of the caller, this list can contain null, and those will be silently ignored. * @since 1.311 * @return - * null if this task is already in the queue and therefore the add operation was no-op. - * Otherwise indicates the {@link WaitingItem} object added, although the nature of the queue + * {@link ScheduleResult.Refused} if Jenkins refused to add this task into the queue (for example because the system + * is about to shutdown.) Otherwise the task is either merged into existing items in the queue + * (in which case you get {@link ScheduleResult.Existing} instance back), or a new item + * gets created in the queue (in which case you get {@link Created}. + * + * Note the nature of the queue * is that such {@link Item} only captures the state of the item at a particular moment, * and by the time you inspect the object, some of its information can be already stale. * - * That said, one can still look at {@link WaitingItem#future}, {@link WaitingItem#id}, etc. + * That said, one can still look at {@link Item#future}, {@link Item#id}, etc. */ - public synchronized WaitingItem schedule(Task p, int quietPeriod, List actions) { + public synchronized @Nonnull ScheduleResult schedule2(Task p, int quietPeriod, List actions) { // remove nulls actions = new ArrayList(actions); for (Iterator itr = actions.iterator(); itr.hasNext();) { @@ -500,7 +514,7 @@ public class Queue extends ResourceController implements Saveable { for(QueueDecisionHandler h : QueueDecisionHandler.all()) if (!h.shouldSchedule(p, actions)) - return null; // veto + return ScheduleResult.refused(); // veto return scheduleInternal(p, quietPeriod, actions); } @@ -517,7 +531,7 @@ public class Queue extends ResourceController implements Saveable { * * That said, one can still look at {@link WaitingItem#future}, {@link WaitingItem#id}, etc. */ - private synchronized WaitingItem scheduleInternal(Task p, int quietPeriod, List actions) { + private synchronized @Nonnull ScheduleResult scheduleInternal(Task p, int quietPeriod, List actions) { Calendar due = new GregorianCalendar(); due.add(Calendar.SECOND, quietPeriod); @@ -542,7 +556,7 @@ public class Queue extends ResourceController implements Saveable { WaitingItem added = new WaitingItem(due,p,actions); added.enter(this); scheduleMaintenance(); // let an executor know that a new item is in the queue. - return added; + return ScheduleResult.created(added); } LOGGER.log(Level.FINE, "{0} is already in the queue", p); @@ -576,7 +590,12 @@ public class Queue extends ResourceController implements Saveable { } if (queueUpdated) scheduleMaintenance(); - return null; + + // REVISIT: when there are multiple existing items in the queue that matches the incoming one, + // whether the new one should affect all existing ones or not is debateable. I for myself + // thought this would only affect one, so the code was bit of surprise, but I'm keeping the current + // behaviour. + return ScheduleResult.existing(duplicatesInQueue.get(0)); } @@ -604,7 +623,14 @@ public class Queue extends ResourceController implements Saveable { * Convenience wrapper method around {@link #schedule(Task, int, List)} */ public synchronized WaitingItem schedule(Task p, int quietPeriod, Action... actions) { - return schedule(p, quietPeriod, Arrays.asList(actions)); + return schedule2(p, quietPeriod, actions).getCreateItem(); + } + + /** + * Convenience wrapper method around {@link #schedule2(Task, int, List)} + */ + public synchronized @Nonnull ScheduleResult schedule2(Task p, int quietPeriod, Action... actions) { + return schedule2(p, quietPeriod, Arrays.asList(actions)); } /** diff --git a/core/src/main/java/hudson/model/queue/ScheduleResult.java b/core/src/main/java/hudson/model/queue/ScheduleResult.java new file mode 100644 index 0000000000..680cd77fe5 --- /dev/null +++ b/core/src/main/java/hudson/model/queue/ScheduleResult.java @@ -0,0 +1,111 @@ +package hudson.model.queue; + +import hudson.model.Action; +import hudson.model.Queue; +import hudson.model.Queue.Item; +import hudson.model.Queue.Task; +import hudson.model.Queue.WaitingItem; + +/** + * Result of {@link Queue#schedule2} + * + * @author Kohsuke Kawaguchi + * @since 1.521 + * @see Queue#schedule(Task, int, Action...) + */ +public abstract class ScheduleResult { + + /** + * If true, the {@link #getItem()} is newly created + * as a result of {@link Queue#schedule2}. + */ + public boolean isCreated() { + return false; + } + + /** + * The scheduling of the task was refused and the queue didn't change. + * If this method returns true, {@link #getItem()} will return null. + */ + public boolean isRefused() { + return false; + } + + /** + * Unless {@link #isRefused()} is true, this method either returns + * the newly created item in the queue or the existing item that's already + * in the queue that matched the submitted task. + */ + public Item getItem() { + return null; + } + + /** + * If {@link #isCreated()} returns true, then this method returns + * the newly created item, which is always of the type {@link WaitingItem}. + */ + public WaitingItem getCreateItem() { + return null; + } + + /** + * Opposite of {@link #isRefused()} + */ + public final boolean isAccepted() { + return !isRefused(); + } + + public static final class Created extends ScheduleResult { + private final WaitingItem item; + private Created(WaitingItem item) { + this.item = item; + } + + @Override + public boolean isCreated() { + return true; + } + + @Override + public WaitingItem getCreateItem() { + return item; + } + + @Override + public Item getItem() { + return item; + } + } + + public static final class Existing extends ScheduleResult { + private final Item item; + + private Existing(Item item) { + this.item = item; + } + + @Override + public Item getItem() { + return item; + } + } + + public static final class Refused extends ScheduleResult { + @Override + public boolean isRefused() { + return true; + } + } + + public static Created created(WaitingItem i) { + return new Created(i); + } + + public static Existing existing(Item i) { + return new Existing(i); + } + + public static Refused refused() { + return new Refused(); + } +} diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy index 4cd88bd731..76d6858232 100644 --- a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy @@ -23,7 +23,8 @@ */ package hudson.model; -import com.gargoylesoftware.htmlunit.ElementNotFoundException; +import com.gargoylesoftware.htmlunit.ElementNotFoundException +import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; import com.gargoylesoftware.htmlunit.HttpMethod; import com.gargoylesoftware.htmlunit.WebRequestSettings; import com.gargoylesoftware.htmlunit.html.HtmlForm; @@ -69,7 +70,7 @@ import org.jvnet.hudson.test.MockFolder; * @author Kohsuke Kawaguchi */ public class AbstractProjectTest extends HudsonTestCase { - public void testConfigRoundtrip() throws Exception { + public void testConfigRoundtrip() { def project = createFreeStyleProject(); def l = jenkins.getLabel("foo && bar"); project.assignedLabel = l; @@ -81,7 +82,7 @@ public class AbstractProjectTest extends HudsonTestCase { /** * Tests the workspace deletion. */ - public void testWipeWorkspace() throws Exception { + public void testWipeWorkspace() { def project = createFreeStyleProject(); project.buildersList.add(new Shell("echo hello")); @@ -98,7 +99,7 @@ public class AbstractProjectTest extends HudsonTestCase { * Makes sure that the workspace deletion is protected. */ @PresetData(DataSet.NO_ANONYMOUS_READACCESS) - public void testWipeWorkspaceProtected() throws Exception { + public void testWipeWorkspaceProtected() { def project = createFreeStyleProject(); project.getBuildersList().add(new Shell("echo hello")); @@ -115,7 +116,7 @@ public class AbstractProjectTest extends HudsonTestCase { * when the user doesn't have an access. */ @PresetData(DataSet.ANONYMOUS_READONLY) - public void testWipeWorkspaceProtected2() throws Exception { + public void testWipeWorkspaceProtected2() { ((GlobalMatrixAuthorizationStrategy) jenkins.getAuthorizationStrategy()).add(AbstractProject.WORKSPACE,"anonymous"); // make sure that the deletion is protected in the same way @@ -138,7 +139,7 @@ public class AbstractProjectTest extends HudsonTestCase { /** * Tests the <optionalBlock @field> round trip behavior by using {@link AbstractProject#concurrentBuild} */ - public void testOptionalBlockDataBindingRoundtrip() throws Exception { + public void testOptionalBlockDataBindingRoundtrip() { def p = createFreeStyleProject(); [true,false].each { b -> p.concurrentBuild = b; @@ -151,7 +152,7 @@ public class AbstractProjectTest extends HudsonTestCase { * Tests round trip configuration of the blockBuildWhenUpstreamBuilding field */ @Bug(4423) - public void testConfiguringBlockBuildWhenUpstreamBuildingRoundtrip() throws Exception { + public void testConfiguringBlockBuildWhenUpstreamBuildingRoundtrip() { def p = createFreeStyleProject(); p.blockBuildWhenUpstreamBuilding = false; @@ -173,7 +174,7 @@ public class AbstractProjectTest extends HudsonTestCase { * to avoid allocating unnecessary workspaces. */ @Bug(4202) - public void testPollingAndBuildExclusion() throws Exception { + public void testPollingAndBuildExclusion() { final OneShotEvent sync = new OneShotEvent(); final FreeStyleProject p = createFreeStyleProject(); @@ -224,7 +225,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(1986) - public void testBuildSymlinks() throws Exception { + public void testBuildSymlinks() { // If we're on Windows, don't bother doing this. if (Functions.isWindows()) return; @@ -261,7 +262,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(2543) - public void testSymlinkForPostBuildFailure() throws Exception { + public void testSymlinkForPostBuildFailure() { // If we're on Windows, don't bother doing this. if (Functions.isWindows()) return; @@ -286,7 +287,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(15156) - public void testGetBuildAfterGC() throws Exception { + public void testGetBuildAfterGC() { FreeStyleProject job = createFreeStyleProject(); job.scheduleBuild2(0, new Cause.UserIdCause()).get(); jenkins.queue.clearLeftItems(); @@ -295,7 +296,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(13502) - public void testHandleBuildTrigger() throws Exception { + public void testHandleBuildTrigger() { Project u = createFreeStyleProject("u"), d = createFreeStyleProject("d"), e = createFreeStyleProject("e"); @@ -330,7 +331,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(17137) - public void testExternalBuildDirectorySymlinks() throws Exception { + public void testExternalBuildDirectorySymlinks() { // TODO when using JUnit 4 add: Assume.assumeFalse(Functions.isWindows()); // symlinks may not be available def form = createWebClient().goTo("configure").getFormByName("config"); def builds = createTmpDir(); @@ -361,7 +362,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(17138) - public void testExternalBuildDirectoryRenameDelete() throws Exception { + public void testExternalBuildDirectoryRenameDelete() { def form = createWebClient().goTo("configure").getFormByName("config"); def builds = createTmpDir(); form.getInputByName("_.rawBuildsDir").setValueAttribute(builds.toString() + "/\${ITEM_FULL_NAME}"); @@ -381,7 +382,7 @@ public class AbstractProjectTest extends HudsonTestCase { } @Bug(17575) - public void testDeleteRedirect() throws Exception { + public void testDeleteRedirect() { createFreeStyleProject("j1"); assert "" == deleteRedirectTarget("job/j1"); createFreeStyleProject("j2"); @@ -394,12 +395,58 @@ public class AbstractProjectTest extends HudsonTestCase { assert "job/d/view/v2/" == deleteRedirectTarget("job/d/view/v2/job/j4"); assert "view/v1/job/d/" == deleteRedirectTarget("view/v1/job/d/job/j5"); } - private String deleteRedirectTarget(String job) throws Exception { - WebClient wc = createWebClient(); + + private String deleteRedirectTarget(String job) { + def wc = createWebClient(); String base = wc.getContextPath(); String loc = wc.getPage(wc.addCrumb(new WebRequestSettings(new URL(base + job + "/doDelete"), HttpMethod.POST))).getWebResponse().getUrl().toString(); assert loc.startsWith(base): loc; return loc.substring(base.length()); } + @Bug(18407) + public void testQueueSuccessBehavior() { + // prevent any builds to test the behaviour + jenkins.numExecutors = 0; + jenkins.updateComputerList(false); + + def p = createFreeStyleProject() + def f = p.scheduleBuild2(0) + assert f!=null; + def g = p.scheduleBuild2(0) + assert f==g; + + p.makeDisabled(true) + assert p.scheduleBuild2(0)==null + } + + /** + * Do the same as {@link #testQueueSuccessBehavior()} but over HTTP + */ + @Bug(18407) + public void testQueueSuccessBehaviorOverHTTP() { + // prevent any builds to test the behaviour + jenkins.numExecutors = 0; + jenkins.updateComputerList(false); + + def p = createFreeStyleProject() + def wc = createWebClient(); + + def rsp = wc.getPage("${getURL()}${p.url}build").webResponse + assert rsp.statusCode==201; + assert rsp.getResponseHeaderValue("Location")!=null; + + def rsp2 = wc.getPage("${getURL()}${p.url}build").webResponse + assert rsp2.statusCode==201; + assert rsp.getResponseHeaderValue("Location")==rsp2.getResponseHeaderValue("Location") + + p.makeDisabled(true) + + try { + wc.getPage("${getURL()}${p.url}build") + fail(); + } catch (FailingHttpStatusCodeException e) { + // request should fail + } + } } -- GitLab