try to lock as few resources as possible

adding a struct class for better code clarity

Author: dcendents

src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

@@ -34,6 +34,7 @@
 import net.sf.json.JSONObject;
 
 import org.apache.commons.lang.StringUtils;
+import org.jenkins.plugins.lockableresources.queue.LockableResourcesCandidatesStruct;
 import org.jenkins.plugins.lockableresources.queue.LockableResourcesStruct;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript;
 import org.jenkins.plugins.lockableresources.queue.QueuedContextStruct;
@@ -638,9 +639,9 @@ public boolean configure(StaplerRequest req, JSONObject json)
 	public synchronized Set<LockableResource> checkResourcesAvailability(List<LockableResourcesStruct> requiredResourcesList,
 			@Nullable PrintStream logger, @Nullable List<String> lockedResourcesAboutToBeUnlocked) {
 
-		// Build possible resources for each requirement
-		Map<List<LockableResource>, Integer> allCandidates = new HashMap<>();
+		List<LockableResourcesCandidatesStruct> requiredResourcesCandidatesList = new ArrayList<>();
 
+		// Build possible resources for each requirement
 		for (LockableResourcesStruct requiredResources : requiredResourcesList) {
 			// get possible resources
 			int requiredAmount = 0; // 0 means all
@@ -662,25 +663,21 @@ public synchronized Set<LockableResource> checkResourcesAvailability(List<Lockab
 				requiredAmount = candidates.size();
 			}
 
-			allCandidates.put(candidates, requiredAmount);
+			requiredResourcesCandidatesList.add(new LockableResourcesCandidatesStruct(candidates, requiredAmount));
 		}
 
 		// Process freed resources
-		Map<Map.Entry<List<LockableResource>, Integer>, List<LockableResource>> currentSelection = new HashMap<>();
 		int totalSelected = 0;
 
-		for (Map.Entry<List<LockableResource>, Integer> entry : allCandidates.entrySet()) {
-			List<LockableResource> candidates = entry.getKey();
-			int requiredAmount = entry.getValue();
-			
+		for (LockableResourcesCandidatesStruct requiredResources : requiredResourcesCandidatesList) {
 			// start with an empty set of selected resources
 			List<LockableResource> selected = new ArrayList<LockableResource>();
 
 			// some resources might be already locked, but will be freed.
 			// Determine if these resources can be reused
 			if (lockedResourcesAboutToBeUnlocked != null) {
-				for (LockableResource candidate : candidates) {
-					if (selected.size() >= requiredAmount) {
+				for (LockableResource candidate : requiredResources.candidates) {
+					if (selected.size() >= requiredResources.requiredAmount) {
 						break;
 					}
 					if (lockedResourcesAboutToBeUnlocked.contains(candidate.getName())) {
@@ -690,7 +687,7 @@ public synchronized Set<LockableResource> checkResourcesAvailability(List<Lockab
 			}
 
 			totalSelected += selected.size();
-			currentSelection.put(entry, selected);
+			requiredResources.selected = selected;
 		}
 
 		// if none of the currently locked resources can be reused,
@@ -702,11 +699,24 @@ public synchronized Set<LockableResource> checkResourcesAvailability(List<Lockab
 		// Find remaining resources
 		Set<LockableResource> allSelected = new HashSet<>();
 
-		for (Map.Entry<Map.Entry<List<LockableResource>, Integer>, List<LockableResource>> entry : currentSelection.entrySet()) {
-			List<LockableResource> candidates = entry.getKey().getKey();
-			List<LockableResource> selected = entry.getValue();
-			int requiredAmount = entry.getKey().getValue();
+		for (LockableResourcesCandidatesStruct requiredResources : requiredResourcesCandidatesList) {
+			List<LockableResource> candidates = requiredResources.candidates;
+			List<LockableResource> selected = requiredResources.selected;
+			int requiredAmount = requiredResources.requiredAmount;
+			
+			// Try and re-use as many previously selected resources first
+			List<LockableResource> alreadySelectedCandidates = new ArrayList<>(candidates);
+			alreadySelectedCandidates.retainAll(allSelected);
+			for (LockableResource rs : alreadySelectedCandidates) {
+				if (selected.size() >= requiredAmount) {
+					break;
+				}
+				if (!rs.isReserved() && !rs.isLocked()) {
+					selected.add(rs);
+				}
+			}
 
+			candidates.removeAll(alreadySelectedCandidates);
 			for (LockableResource rs : candidates) {
 				if (selected.size() >= requiredAmount) {
 					break;

src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesCandidatesStruct.java

@@ -0,0 +1,17 @@
+package org.jenkins.plugins.lockableresources.queue;
+
+import java.util.List;
+
+import org.jenkins.plugins.lockableresources.LockableResource;
+
+public class LockableResourcesCandidatesStruct {
+
+	public List<LockableResource> candidates;
+	public int requiredAmount;
+	public List<LockableResource> selected;
+
+	public LockableResourcesCandidatesStruct(List<LockableResource> candidates, int requiredAmount) {
+		this.candidates = candidates;
+		this.requiredAmount = requiredAmount;
+	}
+}

src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java

@@ -1076,4 +1076,61 @@ public void evaluate() throws Throwable {
 			}
 		});
 	}
+
+	@Test
+	public void lockWithLabelAndLabeledResourceQuantity() {
+		story.addStep(new Statement() {
+			@Override
+			public void evaluate() throws Throwable {
+				LockableResourcesManager.get().createResourceWithLabel("resource1", "label1");
+				LockableResourcesManager.get().createResourceWithLabel("resource2", "label1");
+				LockableResourcesManager.get().createResourceWithLabel("resource3", "label1");
+				LockableResourcesManager.get().createResourceWithLabel("resource4", "label1");
+				WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+				p.setDefinition(new CpsFlowDefinition(
+						"lock(resource: 'resource4', extra: [[resource: 'resource2'], [label: 'label1', quantity: 2]]) {\n" +
+						"	semaphore 'wait-inside'\n" +
+						"}\n" +
+						"echo 'Finish'"
+				));
+				// #1 should lock as few resources as possible
+				WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
+				SemaphoreStep.waitForStart("wait-inside/1", b1);
+
+				WorkflowJob p2 = story.j.jenkins.createProject(WorkflowJob.class, "p2");
+				p2.setDefinition(new CpsFlowDefinition(
+						"lock(label: 'label1', quantity: 3) {\n" +
+						"	semaphore 'wait-inside-quantity3'\n" +
+						"}\n" +
+						"echo 'Finish'"
+				));
+				WorkflowRun b2 = p2.scheduleBuild2(0).waitForStart();
+				story.j.waitForMessage("[Label: label1, Quantity: 3] is locked, waiting...", b2);
+				story.j.waitForMessage("Found 2 available resource(s). Waiting for correct amount: 3.", b2);
+				
+				WorkflowJob p3 = story.j.jenkins.createProject(WorkflowJob.class, "p3");
+				p3.setDefinition(new CpsFlowDefinition(
+						"lock(label: 'label1', quantity: 2) {\n" +
+						"	semaphore 'wait-inside-quantity2'\n" +
+						"}\n" +
+						"echo 'Finish'"
+				));
+				WorkflowRun b3 = p3.scheduleBuild2(0).waitForStart();
+				// While 2 continues waiting, 3 can continue directly
+				SemaphoreStep.waitForStart("wait-inside-quantity2/1", b3);
+				// Let 3 finish
+				SemaphoreStep.success("wait-inside-quantity2/1", null);
+				story.j.waitForMessage("Finish", b3);
+
+				// Unlock resources
+				SemaphoreStep.success("wait-inside/1", null);
+				story.j.waitForMessage("Lock released on resource [{resource4},{resource2},{Label: label1, Quantity: 2},]", b1);
+
+				// #2 gets the lock
+				story.j.waitForMessage("Lock acquired on [Label: label1, Quantity: 3]", b2);
+				SemaphoreStep.success("wait-inside-quantity3/1", null);
+				story.j.waitForMessage("Finish", b2);
+			}
+		});
+	}
 }