ConcurrentModificationException-at-LockedResourcesBuildAction (#626)

* NPE during the release of a lock * API-properties * Using many very fast parallel locks in the same job may raise ConcurrentModificationException * chaosOnRestart
jenkinsci · Feb 16, 2024 · 43673aa · 43673aa
1 parent 6fc1f4c
commit 43673aa
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 1 deletion.
diff --git a/src/main/java/org/jenkins/plugins/lockableresources/actions/LockedResourcesBuildAction.java b/src/main/java/org/jenkins/plugins/lockableresources/actions/LockedResourcesBuildAction.java
@@ -89,7 +89,9 @@ public static void updateAction(Run<?, ?> build, List<String> resourceNames) {
     // -------------------------------------------------------------------------
     /** Add the resource to build action.*/
     @Restricted(NoExternalUse.class)
-    private void add(ResourcePOJO r) {
+    // since the list *this.lockedResources* might be updated from multiple (parallel)
+    // stages, this operation need to be synchronized
+    private synchronized void add(ResourcePOJO r) {
         for (ResourcePOJO pojo : this.lockedResources) {
             if (pojo.getName().equals(r.getName())) {
                 pojo.inc();

diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepWithRestartTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepWithRestartTest.java
@@ -190,4 +190,62 @@ public void checkQueueAfterRestart() throws Throwable {
             j.waitForMessage("Lock released on resource [Label: label, Quantity: 1]", b1);
         });
     }
+
+    @Test
+    public void chaosOnRestart() throws Throwable {
+        final int resourceCount = 50;
+        sessions.then(j -> {
+            for (int i = 1; i <= resourceCount; i++) {
+                LockableResourcesManager.get().createResource("resource" + i);
+            }
+            WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition(
+                    "def stages = [:];\n"
+                            + "for (int i = 1; i <= " + resourceCount + "; i++) {\n"
+                            + "  def stageName = 'order' + i;\n"
+                            + "  def resourceName = 'resource' + i;\n"
+                            + "  def semId = 'wait-inside-lockOrderRestart-' + i;\n"
+                            + "  stages[stageName] = {\n"
+                            + "    lock('ephemeral_' + env.BUILD_NUMBER + '_' + resourceName) {\n"
+                            + "      lock(resourceName) {semaphore semId}\n"
+                            + "    }\n"
+                            + "  }\n"
+                            + "}\n"
+                            + "parallel stages\n"
+                            + "echo 'Finish'",
+                    true));
+            WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
+            SemaphoreStep.waitForStart("wait-inside-lockOrderRestart-" + resourceCount + "/1", b1);
+            WorkflowRun b2 = p.scheduleBuild2(0).waitForStart();
+            // Ensure that b2 reaches the lock before b3
+            j.waitForMessage("[resource" + resourceCount + "] is locked", b2);
+            WorkflowRun b3 = p.scheduleBuild2(0).waitForStart();
+            // Both 2 and 3 are waiting for locking resource1
+            j.waitForMessage("[resource" + resourceCount + "] is locked", b3);
+        });
+
+        sessions.then(j -> {
+            WorkflowJob p = j.jenkins.getItemByFullName("p", WorkflowJob.class);
+            WorkflowRun b1 = p.getBuildByNumber(1);
+            WorkflowRun b2 = p.getBuildByNumber(2);
+            WorkflowRun b3 = p.getBuildByNumber(3);
+
+            // Unlock resources
+            for (int i = 1; i <= resourceCount; i++) {
+                SemaphoreStep.success("wait-inside-lockOrderRestart-" + i + "/1", null);
+            }
+            j.waitForMessage("Lock released on resource [resource" + resourceCount + "]", b1);
+            j.waitForMessage("Lock acquired on [resource" + resourceCount + "]", b2);
+            j.assertLogContains("[resource" + resourceCount + "] is locked by build " + b1.getFullDisplayName(), b3);
+            for (int i = 1; i <= resourceCount; i++) {
+                SemaphoreStep.success("wait-inside-lockOrderRestart-" + i + "/2", null);
+            }
+            SemaphoreStep.waitForStart("wait-inside-lockOrderRestart-" + resourceCount + "/3", b3);
+            j.assertLogContains("Lock acquired on [resource" + resourceCount + "]", b3);
+            for (int i = 1; i <= resourceCount; i++) {
+                SemaphoreStep.success("wait-inside-lockOrderRestart-" + i + "/3", null);
+            }
+            j.waitForMessage("Finish", b3);
+        });
+    }
 }