diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index f164595557..cbc2e4081d 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -102,7 +102,6 @@ import java.util.TreeSet; import java.util.concurrent.Callable; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.Condition; import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; @@ -113,6 +112,7 @@ import javax.servlet.http.HttpServletResponse; import jenkins.model.Jenkins; import jenkins.model.queue.AsynchronousExecution; import jenkins.model.queue.CompositeCauseOfBlockage; +import jenkins.model.queue.QueueIdStrategy; import jenkins.model.queue.QueueItem; import jenkins.security.QueueItemAuthenticator; import jenkins.security.QueueItemAuthenticatorProvider; @@ -124,6 +124,7 @@ import jenkins.util.Timer; import net.jcip.annotations.GuardedBy; import org.jenkinsci.remoting.RoleChecker; import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.Beta; import org.kohsuke.accmod.restrictions.DoNotUse; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.HttpResponse; @@ -369,9 +370,20 @@ public class Queue extends ResourceController implements Saveable { /** * Simple queue state persistence object. */ - static class State { - public long counter; - public List items = new ArrayList<>(); + @Restricted(Beta.class) + public static final class State { + public /* almost final */ List items = new ArrayList<>(); + public /* almost final */ Map properties = new HashMap<>(); + + private Object readResolve() { + if (items == null) { + items = new ArrayList<>(); + } + if (properties == null) { + properties = new HashMap<>(); + } + return this; + } } /** @@ -391,21 +403,18 @@ public class Queue extends ResourceController implements Saveable { Object unmarshaledObj = new XmlFile(XSTREAM, queueFile).read(); List items; + State state; if (unmarshaledObj instanceof State) { - State state = (State) unmarshaledObj; + state = (State) unmarshaledObj; items = state.items; - WaitingItem.COUNTER.set(state.counter); } else { // backward compatibility - it's an old List queue.xml items = (List) unmarshaledObj; - long maxId = 0; - for (Object o : items) { - if (o instanceof Item) { - maxId = Math.max(maxId, ((Item) o).id); - } - } - WaitingItem.COUNTER.set(maxId); + state = new State(); + state.items.addAll(items); } + QueueIdStrategy.get().load(state); + for (Object o : items) { if (o instanceof Task) { @@ -460,8 +469,7 @@ public class Queue extends ResourceController implements Saveable { try { // write out the queue state we want to save State state = new State(); - state.counter = WaitingItem.COUNTER.longValue(); - + QueueIdStrategy.get().persist(state); // write out the tasks on the queue for (Item item : getItems()) { if (item.task instanceof TransientTask) continue; @@ -2532,8 +2540,6 @@ public class Queue extends ResourceController implements Saveable { * {@link Item} in the {@link Queue#waitingList} stage. */ public static final class WaitingItem extends Item implements Comparable { - private static final AtomicLong COUNTER = new AtomicLong(0); - /** * This item can be run after this time. */ @@ -2541,14 +2547,10 @@ public class Queue extends ResourceController implements Saveable { public Calendar timestamp; public WaitingItem(Calendar timestamp, Task project, List actions) { - super(project, actions, COUNTER.incrementAndGet(), new FutureImpl(project)); + super(project, actions, QueueIdStrategy.get().generateIdFor(project, actions), new FutureImpl(project)); this.timestamp = timestamp; } - static int getCurrentCounterValue() { - return COUNTER.intValue(); - } - @Override public int compareTo(WaitingItem that) { int r = this.timestamp.getTime().compareTo(that.timestamp.getTime()); diff --git a/core/src/main/java/jenkins/model/queue/QueueIdStrategy.java b/core/src/main/java/jenkins/model/queue/QueueIdStrategy.java new file mode 100644 index 0000000000..4010ca61c2 --- /dev/null +++ b/core/src/main/java/jenkins/model/queue/QueueIdStrategy.java @@ -0,0 +1,86 @@ +package jenkins.model.queue; + +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.ExtensionList; +import hudson.ExtensionPoint; +import hudson.model.Action; +import hudson.model.Queue; +import java.util.Comparator; +import java.util.List; +import java.util.concurrent.atomic.AtomicLong; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.Beta; +import org.kohsuke.accmod.restrictions.DoNotUse; + +/** + * Pluggable strategy to generate queue item IDs as well as persist an optional opaque state whenever the queue is persisted. + */ +@Restricted(Beta.class) +public abstract class QueueIdStrategy implements ExtensionPoint { + private static final QueueIdStrategy DEFAULT = new DefaultStrategy(); + + public static QueueIdStrategy get() { + var strategies = ExtensionList.lookup(QueueIdStrategy.class); + if (strategies.isEmpty()) { + return DEFAULT; + } + return strategies.get(0); + } + + /** + * Persist the state of this strategy. + */ + public void persist(@NonNull Queue.State queueState) {} + + /** + * Loads the state of this strategy from a persisted queue state. + */ + public void load(@NonNull Queue.State queueState) {} + + /** + * Generates a new ID for the given project and actions. + * @param project The task to be queued. + * @param actions The actions linked the task. + * @return a new queue ID. + */ + public abstract long generateIdFor(@NonNull Queue.Task project, @NonNull List actions); + + /** + * Default implementation if no extension is found. Simply uses a counter. + */ + public static final class DefaultStrategy extends QueueIdStrategy { + private static final AtomicLong COUNTER = new AtomicLong(0); + + @Override + public long generateIdFor(Queue.Task project, List actions) { + return COUNTER.incrementAndGet(); + } + + @Override + public void persist(Queue.State queueState) { + queueState.properties.put(getClass().getName(), COUNTER.get()); + } + + @Override + public void load(Queue.State queueState) { + var prop = queueState.properties.get(getClass().getName()); + if (prop instanceof Long) { + COUNTER.set((Long) prop); + } else { + queueState.items.stream() + .filter(Queue.Item.class::isInstance) + .map(Queue.Item.class::cast) + .max(Comparator.comparing(Queue.Item::getId)) + .ifPresentOrElse( + maxItem -> COUNTER.set(maxItem.getId()), + () -> COUNTER.set(0) + ); + } + } + + @Restricted(DoNotUse.class) // testing only + public static long getCurrentCounterValue() { + return COUNTER.get(); + } + } +} diff --git a/core/src/test/java/hudson/model/QueueTest.java b/core/src/test/java/hudson/model/QueueTest.java index 43b4c8be29..1a338371ff 100644 --- a/core/src/test/java/hudson/model/QueueTest.java +++ b/core/src/test/java/hudson/model/QueueTest.java @@ -36,9 +36,9 @@ public class QueueTest { public void cancelItemOnaValidItemShouldReturnA204() throws IOException, ServletException { when(task.hasAbortPermission()).thenReturn(true); Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH); - queue.schedule(task, 6000); + long id = queue.schedule(task, 6000).getId(); - HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue()); + HttpResponse httpResponse = queue.doCancelItem(id); httpResponse.generateResponse(null, resp, null); verify(resp).setStatus(HttpServletResponse.SC_NO_CONTENT); @@ -48,9 +48,9 @@ public class QueueTest { @Test public void cancelItemOnANonExistingItemShouldReturnA404() throws IOException, ServletException { Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH); - queue.schedule(task, 6000); + long id = queue.schedule(task, 6000).getId(); - HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue() + 1); + HttpResponse httpResponse = queue.doCancelItem(id + 1); httpResponse.generateResponse(null, resp, null); verify(resp).setStatus(HttpServletResponse.SC_NOT_FOUND); @@ -61,9 +61,9 @@ public class QueueTest { public void cancelItemOnANonCancellableItemShouldReturnA422() throws IOException, ServletException { when(task.hasAbortPermission()).thenReturn(false); Queue queue = new Queue(LoadBalancer.CONSISTENT_HASH); - queue.schedule(task, 6000); + long id = queue.schedule(task, 6000).getId(); - HttpResponse httpResponse = queue.doCancelItem(Queue.WaitingItem.getCurrentCounterValue()); + HttpResponse httpResponse = queue.doCancelItem(id); httpResponse.generateResponse(null, resp, null); verify(resp).setStatus(422); diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 5e52bf6416..878b04e28d 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -108,6 +108,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import jenkins.model.BlockedBecauseOfBuildInProgress; import jenkins.model.Jenkins; +import jenkins.model.queue.QueueIdStrategy; import jenkins.security.QueueItemAuthenticatorConfiguration; import org.acegisecurity.acls.sid.PrincipalSid; import org.apache.commons.fileupload.FileUploadException; @@ -200,7 +201,7 @@ public class QueueTest { // The current counter should be the id from the item brought back // from the persisted queue.xml. - assertEquals(3, Queue.WaitingItem.getCurrentCounterValue()); + assertEquals(3, QueueIdStrategy.DefaultStrategy.getCurrentCounterValue()); // Clear the queue assertTrue(r.jenkins.getQueue().cancel(r.jenkins.getItemByFullName("test", FreeStyleProject.class))); @@ -213,7 +214,7 @@ public class QueueTest { Queue q = r.jenkins.getQueue(); resetQueueState(); - assertEquals(0, Queue.WaitingItem.getCurrentCounterValue()); + assertEquals(0, QueueIdStrategy.DefaultStrategy.getCurrentCounterValue()); // prevent execution to push stuff into the queue r.jenkins.setNumExecutors(0); @@ -234,7 +235,7 @@ public class QueueTest { assertEquals(0, q.getItems().length); // The counter state should be maintained. - assertEquals(1, Queue.WaitingItem.getCurrentCounterValue()); + assertEquals(1, QueueIdStrategy.DefaultStrategy.getCurrentCounterValue()); } /** diff --git a/test/src/test/java/jenkins/model/ParameterizedJobMixInTest.java b/test/src/test/java/jenkins/model/ParameterizedJobMixInTest.java index f4b11cb2ea..f873a69d30 100644 --- a/test/src/test/java/jenkins/model/ParameterizedJobMixInTest.java +++ b/test/src/test/java/jenkins/model/ParameterizedJobMixInTest.java @@ -25,6 +25,7 @@ package jenkins.model; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertThrows; @@ -90,11 +91,12 @@ public class ParameterizedJobMixInTest { webClient.getPage(webClient.addCrumb(new WebRequest(new URL(j.getURL(), project.getUrl() + "build"), HttpMethod.POST))); long triggerTime = System.currentTimeMillis(); - Queue.Item item = Jenkins.get().getQueue().getItem(1); - assertThat(item, instanceOf(Queue.WaitingItem.class)); - assertThat(item.task, instanceOf(FreeStyleProject.class)); + Queue.Item[] items = Jenkins.get().getQueue().getItems(); + assertThat(items, arrayWithSize(1)); + assertThat(items[0], instanceOf(Queue.WaitingItem.class)); + assertThat(items[0].task, instanceOf(FreeStyleProject.class)); - Queue.WaitingItem waitingItem = (Queue.WaitingItem) item; + Queue.WaitingItem waitingItem = (Queue.WaitingItem) items[0]; Assert.assertTrue(waitingItem.timestamp.getTimeInMillis() - triggerTime > 45000); Jenkins.get().getQueue().doCancelItem(1);