[FIXED JENKINS-34433] Signal queued Pipeline tasks on unreserve

Previously, we had already been signalling queued Pipeline tasks on unlocking, but not on unreserving. So...let's do that.
jenkinsci · Aug 15, 2017 · d18f7ba · d18f7ba
1 parent 3099e8b
commit d18f7ba
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 1 deletion.
diff --git a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java
@@ -501,9 +501,73 @@ public synchronized boolean reserve(List<LockableResource> resources,
 		return true;
 	}
 
+	private void unreserveResources(@Nonnull List<LockableResource> resources) {
+		for (LockableResource l : resources) {
+			l.unReserve();
+		}
+		save();
+	}
 	public synchronized void unreserve(List<LockableResource> resources) {
+		// make sure there is a list of resources to unreserve
+		if (resources == null || (resources.size() == 0)) {
+			return;
+		}
+		List<String> resourceNamesToUnreserve = new ArrayList<>();
 		for (LockableResource r : resources) {
-			r.unReserve();
+			resourceNamesToUnreserve.add(r.getName());
+		}
+
+		// check if there are resources which can be unlocked (and shall not be unlocked)
+		List<LockableResource> requiredResourceForNextContext = null;
+		QueuedContextStruct nextContext = this.getNextQueuedContext(resourceNamesToUnreserve, false);
+
+		// no context is queued which can be started once these resources are free'd.
+		if (nextContext == null) {
+			unreserveResources(resources);
+			return;
+		}
+
+		// remove context from queue and process it
+		requiredResourceForNextContext = checkResourcesAvailability(nextContext.getResources(), null, resourceNamesToUnreserve);
+		this.queuedContexts.remove(nextContext);
+
+		// resourceNamesToUnreserve contains the names of the previous resources.
+		// requiredResourceForNextContext contains the resource objects which are required for the next context.
+		// It is guaranteed that there is an overlap between the two - the resources which are to be reused.
+		boolean needToWait = false;
+		for (LockableResource requiredResource : requiredResourceForNextContext) {
+			if (!resourceNamesToUnreserve.contains(requiredResource.getName())) {
+				if (requiredResource.isReserved() || requiredResource.isLocked()) {
+					needToWait = true;
+					break;
+				}
+			}
+		}
+
+		if (needToWait) {
+			unreserveResources(resources);
+			return;
+		} else {
+			unreserveResources(resources);
+			List<String> resourceNamesToLock = new ArrayList<String>();
+
+			// lock all (old and new resources)
+			for (LockableResource requiredResource : requiredResourceForNextContext) {
+				try {
+					requiredResource.setBuild(nextContext.getContext().get(Run.class));
+					resourceNamesToLock.add(requiredResource.getName());
+				} catch (Exception e) {
+					// skip this context, as the build cannot be retrieved (maybe it was deleted while running?)
+					LOGGER.log(Level.WARNING, "Skipping queued context for lock. Can not get the Run object from the context to proceed with lock, " +
+							"this could be a legitimate status if the build waiting for the lock was deleted or" +
+							" hard killed. More information at Level.FINE if debug is needed.");
+					LOGGER.log(Level.FINE, "Can not get the Run object from the context to proceed with lock", e);
+					return;
+				}
+			}
+
+			// continue with next context
+			LockStepExecution.proceed(resourceNamesToLock, nextContext.getContext(), nextContext.getResourceDescription(), false);
 		}
 		save();
 	}

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
@@ -29,6 +29,7 @@
 import hudson.model.Result;
 
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeFalse;
 
 public class LockStepTest {
@@ -693,6 +694,39 @@ public void evaluate() throws Throwable {
 		});
 	}
 
+	@Issue("JENKINS-34433")
+	@Test
+	public void manualUnreserveUnblocksJob() throws Exception {
+		story.addStep(new Statement() {
+			@Override
+			public void evaluate() throws Throwable {
+				LockableResourcesManager.get().createResource("resource1");
+				JenkinsRule.WebClient wc = story.j.createWebClient();
+
+				wc.goTo("lockable-resources/reserve?resource=resource1");
+				LockableResource resource1 = LockableResourcesManager.get().fromName("resource1");
+				assertNotNull(resource1);
+				resource1.setReservedBy("someone");
+				assertTrue(resource1.isReserved());
+
+				WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+				p.setDefinition(new CpsFlowDefinition("retry(99) {\n" +
+						"    lock('resource1') {\n" +
+						"        semaphore('wait-inside')\n" +
+						"     }\n" +
+						"}", true));
+
+
+				WorkflowRun r = p.scheduleBuild2(0).waitForStart();
+				story.j.waitForMessage("[resource1] is locked, waiting...", r);
+				wc.goTo("lockable-resources/unreserve?resource=resource1");
+				SemaphoreStep.waitForStart("wait-inside/1", r);
+				SemaphoreStep.success("wait-inside/1", null);
+				story.j.assertBuildStatusSuccess(story.j.waitForCompletion(r));
+			}
+		});
+	}
+
 	private void waitAndClear(int semaphoreIndex, List<WorkflowRun> nextRuns) throws Exception {
 		WorkflowRun toClear = nextRuns.get(0);