Merge pull request #825 from jimklimov/JENKINS-76294-CME

Investigate and address `ConcurrentModificationException` which occasionally happens during XStream saving of running workflows [JENKINS-76294]

Author: jimklimov

pom.xml

@@ -62,6 +62,8 @@
     <spotbugs.threshold>Low</spotbugs.threshold>
     <spotless.check.skip>false</spotless.check.skip>
     <ban-junit4-imports.skip>false</ban-junit4-imports.skip>
+    <system-stubs.version>2.1.8</system-stubs.version>
+    <!-- FIXME: Get this into parent BOM? -->
   </properties>
 
   <dependencyManagement>
@@ -110,6 +112,8 @@
       <groupId>org.jenkins-ci.plugins.workflow</groupId>
       <artifactId>workflow-support</artifactId>
     </dependency>
+
+    <!-- Testing scope -->
     <dependency>
       <groupId>io.jenkins</groupId>
       <artifactId>configuration-as-code</artifactId>
@@ -135,8 +139,6 @@
       <artifactId>workflow-job</artifactId>
       <scope>test</scope>
     </dependency>
-
-    <!-- Testing scope -->
     <dependency>
       <groupId>org.jenkins-ci.plugins.workflow</groupId>
       <artifactId>workflow-support</artifactId>
@@ -153,6 +155,14 @@
       <artifactId>mockito-junit-jupiter</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <!-- https://www.baeldung.com/java-system-stubs -->
+      <groupId>uk.org.webcompere</groupId>
+      <artifactId>system-stubs-jupiter</artifactId>
+      <version>${system-stubs.version}</version>
+      <!-- FIXME: Get this into parent BOM? -->
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <repositories>

src/main/java/org/jenkins/plugins/lockableresources/actions/LockedResourcesBuildAction.java

@@ -6,6 +6,7 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.List;
+import net.jcip.annotations.GuardedBy;
 import org.jenkins.plugins.lockableresources.Messages;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -18,8 +19,10 @@
 @Restricted(NoExternalUse.class)
 public class LockedResourcesBuildAction implements Action {
 
+    @GuardedBy("logs")
     private final List<LogEntry> logs = new ArrayList<>();
-    private final transient Object syncLogs = new Object();
+
+    @GuardedBy("resourcesInUse")
     private final List<String> resourcesInUse = new ArrayList<>();
 
     public LockedResourcesBuildAction() {}
@@ -47,13 +50,13 @@ public List<String> getCurrentUsedResourceNames() {
     }
 
     public void addUsedResources(List<String> resourceNames) {
-        synchronized (resourcesInUse) {
+        synchronized (this.resourcesInUse) {
             resourcesInUse.addAll(resourceNames);
         }
     }
 
     public void removeUsedResources(List<String> resourceNames) {
-        synchronized (resourcesInUse) {
+        synchronized (this.resourcesInUse) {
             resourcesInUse.removeAll(resourceNames);
         }
     }
@@ -94,7 +97,6 @@ public static void addLog(
     }
 
     public void addLog(final String resourceName, final String step, final String action) {
-
         synchronized (this.logs) {
             this.logs.add(new LogEntry(step, action, resourceName));
         }
@@ -107,6 +109,37 @@ public List<LogEntry> getReadOnlyLogs() {
         }
     }
 
+    @Restricted(NoExternalUse.class)
+    public List<String> getReadOnlyResourcesInUse() {
+        synchronized (this.resourcesInUse) {
+            return new ArrayList<>(Collections.unmodifiableCollection(this.resourcesInUse));
+        }
+    }
+
+    /** Copy constructor, primarily for {@link #writeReplace} */
+    private LockedResourcesBuildAction(LockedResourcesBuildAction other) {
+        synchronized (other.logs) {
+            synchronized (other.resourcesInUse) {
+                this.logs.addAll(other.getReadOnlyLogs());
+                this.resourcesInUse.addAll(other.getReadOnlyResourcesInUse());
+            }
+        }
+    }
+    /**
+     * Ensure iteration during XStream marshalling is also synchronized,
+     * otherwise we tend to get {@link java.util.ConcurrentModificationException}.<br/>
+     *
+     * The recommended approach is to copy-on-write the properties so a
+     * snapshot can always be scraped consistently. But this can be costly
+     * at run-time, so we use the next-best option: produce a consistent
+     * replica of the current object for actual saving only on demand.<br/>
+     *
+     * This method is found by XStream via reflection.<br/>
+     */
+    protected synchronized Object writeReplace() {
+        return new LockedResourcesBuildAction(this);
+    }
+
     public static class LogEntry {
 
         private final String step;