Fix-possible-npe-in-get-queue (#609)

jenkinsci · Jan 18, 2024 · 512d6c4 · 512d6c4
1 parent 6a4cfee
commit 512d6c4
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 20 deletions.
diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/QueuedContextStruct.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/QueuedContextStruct.java
@@ -89,9 +89,12 @@ public StepContext getContext() {
                 return null;
             }
             return this.getContext().get(Run.class);
-        } catch (IOException | InterruptedException e) {
+        } catch (Exception e) {
             // for some reason there is no Run object for this context
-            LOGGER.log(Level.FINE, "Cannot get the Run object from the context to proceed with lock", e);
+            LOGGER.log(
+                    Level.WARNING,
+                    "Cannot get the build object from the context to proceed with lock. The build probably does not exists (deleted?)",
+                    e);
             return null;
         }
     }

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepHardKillTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepHardKillTest.java
@@ -104,10 +104,7 @@ public void hardKillNewBuildClearsLock() throws Exception {
     public void hardKillWithWaitingRuns() throws Exception {
         LockableResourcesManager.get().createResource("resource1");
         WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
-        p.setDefinition(new CpsFlowDefinition(
-                /// FIXME why we need retry here, when you know it, write the comment here or remove it
-                "retry(99) {\n" + "    lock('resource1') {\n" + "        semaphore('wait-inside')\n" + "     }\n" + "}",
-                true));
+        p.setDefinition(new CpsFlowDefinition("lock('resource1') { semaphore('wait-inside') }", true));
 
         WorkflowRun prevBuild = null;
         for (int i = 0; i < 3; i++) {
@@ -134,14 +131,7 @@ public void hardKillWithWaitingRunsOnLabel() throws Exception {
         LockableResourcesManager.get().createResourceWithLabel("resource1", "label1");
         LockableResourcesManager.get().createResourceWithLabel("resource2", "label1");
         WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
-        p.setDefinition(new CpsFlowDefinition(
-                /// FIXME why we need retry here, when you know it, write the comment here or remove it
-                "retry(99) {\n"
-                        + "    lock(label: 'label1', quantity: 1) {\n"
-                        + "        semaphore('wait-inside')\n"
-                        + "     }\n"
-                        + "}",
-                true));
+        p.setDefinition(new CpsFlowDefinition("lock(label: 'label1', quantity: 1) { semaphore('wait-inside')}", true));
 
         WorkflowRun firstPrev = null;
         WorkflowRun secondPrev = null;

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
@@ -428,12 +428,7 @@ public void deleteRunningBuildNewBuildClearsLock() throws Exception {
     public void unlockButtonWithWaitingRuns() throws Exception {
         LockableResourcesManager.get().createResource("resource1");
         WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
-        p.setDefinition(new CpsFlowDefinition(
-                // "retry(99) {\n"
-                "    lock('resource1') {\n" + "        semaphore('wait-inside')\n" + "     }\n"
-                // + "}"
-                ,
-                true));
+        p.setDefinition(new CpsFlowDefinition("lock('resource1') { semaphore('wait-inside') }", true));
 
         JenkinsRule.WebClient wc = j.createWebClient();
 

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepWithRestartTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepWithRestartTest.java
@@ -1,7 +1,10 @@
 package org.jenkins.plugins.lockableresources;
 
+import static org.junit.Assert.assertFalse;
+
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
+import hudson.security.FullControlOnceLoggedInAuthorizationStrategy;
 import java.util.Collections;
 import java.util.logging.Logger;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
@@ -10,6 +13,7 @@
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.JenkinsSessionRule;
 
 public class LockStepWithRestartTest extends LockStepTestBase {
@@ -145,4 +149,45 @@ public void testReserveOverRestart() throws Throwable {
             j.waitUntilNoActivity();
         });
     }
+
+    @Test
+    public void checkQueueAfterRestart() throws Throwable {
+        sessions.then(j -> {
+            LockableResourcesManager lrm = LockableResourcesManager.get();
+
+            lrm.createResourceWithLabel("resource1", "label");
+            lrm.createResourceWithLabel("resource2", "label");
+            WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition("lock(label: 'label', quantity: 1) { echo 'inside lock' }", true));
+
+            j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+            j.jenkins.setAuthorizationStrategy(new FullControlOnceLoggedInAuthorizationStrategy());
+
+            lrm.reserve(Collections.singletonList(lrm.fromName("resource1")), "test");
+            lrm.reserve(Collections.singletonList(lrm.fromName("resource2")), "test");
+
+            WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
+            j.waitForMessage("The resource [resource1] is reserved by test.", b1);
+        });
+
+        sessions.then(j -> {
+            LockableResourcesManager lrm = LockableResourcesManager.get();
+            WorkflowJob p = j.jenkins.getItemByFullName("p", WorkflowJob.class);
+            WorkflowRun b1 = p.getBuildByNumber(1);
+
+            j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+            j.jenkins.setAuthorizationStrategy(new FullControlOnceLoggedInAuthorizationStrategy());
+
+            JenkinsRule.WebClient wc = j.createWebClient();
+            wc.login("user");
+            TestHelpers.clickButton(wc, "unreserve");
+
+            lrm.unreserve(Collections.singletonList(lrm.fromName("resource1")));
+
+            assertFalse(lrm.fromName("resource1").isReserved());
+
+            j.waitForMessage("Lock acquired on [Label: label, Quantity: 1]", b1);
+            j.waitForMessage("Lock released on resource [Label: label, Quantity: 1]", b1);
+        });
+    }
 }