Synchronise on project

I am not sure this makes any sense. I am just trying to solve this issue: https://issues.jenkins-ci.org/browse/JENKINS-41880 My thoughts: This is likely caused by changing the finger prints, the finger prints are scoped to the project (right?). If this is the case synchronising on the project could make sense. Also `addFromFacet` should not be a performance killer and rather quick. The alternative would be for me to synchronise the complete access to the method. WDYT?
jenkinsci · Apr 3, 2018 · 63f655f · 63f655f
1 parent e00c616
commit 63f655f
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 46 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java
@@ -96,20 +96,20 @@ public Set<String> getImageIDs() {
     public @CheckForNull String getFingerprintHash(@CheckForNull String imageId) {
         return (imageId != null) ? DockerFingerprints.getFingerprintHash(imageId) : null;
     }
-    
+
     @Restricted(NoExternalUse.class)
     public @CheckForNull Fingerprint getFingerprint(@CheckForNull String imageId) {
         if (imageId == null) {
             return null;
         }
-        
+
         try {
             return DockerFingerprints.of(imageId);
         } catch (IOException ex) {
             return null; // nothing to do in web UI - return null as well
         }
     }
-    
+
     public List<DockerFingerprintFacet> getDockerFacets(String imageId) {
         List<DockerFingerprintFacet> res = new LinkedList<DockerFingerprintFacet>();
         final Fingerprint fp = getFingerprint(imageId);
@@ -125,9 +125,9 @@ public List<DockerFingerprintFacet> getDockerFacets(String imageId) {
 
     /**
      * Adds an action with a reference to fingerprint if required. It's
-     * recommended to call the method from {
+     * recommended to call the method from {@link hudson.BulkChange}
+     * transaction to avoid saving the {@link Run} multiple times.
      *
-     * @BulkChange} transaction to avoid saving the {@link Run} multiple times.
      * @param fp Fingerprint
      * @param imageId ID of the docker image
      * @param run Run to be updated
@@ -140,7 +140,7 @@ static void addToRun(Fingerprint fp, String imageId, Run run) throws IOException
                 action = new DockerFingerprintAction();
                 run.addAction(action);
             }
-            
+
             if (action.imageIDs.add(imageId)) {
                 run.save();
             } // else no need to save updates

diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java
@@ -26,19 +26,19 @@
 import hudson.BulkChange;
 import hudson.model.Fingerprint;
 import hudson.model.Run;
+import jenkins.model.FingerprintFacet;
 import jenkins.model.Jenkins;
+import org.apache.commons.lang.StringUtils;
 
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import javax.annotation.CheckForNull;
-import javax.annotation.Nonnull;
-import jenkins.model.FingerprintFacet;
-import org.apache.commons.lang.StringUtils;
 
 /**
  * Entry point into fingerprint related functionalities in Docker.
@@ -254,55 +254,57 @@ public static void addRunFacet(@Nonnull ContainerRecord record, @Nonnull Run<?,?
      * @param run the build in which the image building occurred
      */
     public static void addFromFacet(@CheckForNull String ancestorImageId, @Nonnull String descendantImageId, @Nonnull Run<?,?> run) throws IOException {
-        long timestamp = System.currentTimeMillis();
-        if (ancestorImageId != null) {
-            Fingerprint f = forImage(run, ancestorImageId);
+        synchronized (run.getParent()) {
+            long timestamp = System.currentTimeMillis();
+            if (ancestorImageId != null) {
+                Fingerprint f = forImage(run, ancestorImageId);
+                Collection<FingerprintFacet> facets = f.getFacets();
+                DockerDescendantFingerprintFacet descendantFacet = null;
+                for (FingerprintFacet facet : facets) {
+                    if (facet instanceof DockerDescendantFingerprintFacet) {
+                        descendantFacet = (DockerDescendantFingerprintFacet) facet;
+                        break;
+                    }
+                }
+                BulkChange bc = new BulkChange(f);
+                try {
+                    if (descendantFacet == null) {
+                        descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId);
+                        facets.add(descendantFacet);
+                    }
+                    descendantFacet.addDescendantImageId(descendantImageId);
+                    descendantFacet.addFor(run);
+                    DockerFingerprintAction.addToRun(f, ancestorImageId, run);
+                    bc.commit();
+                } finally {
+                    bc.abort();
+                }
+            }
+            Fingerprint f = forImage(run, descendantImageId);
             Collection<FingerprintFacet> facets = f.getFacets();
-            DockerDescendantFingerprintFacet descendantFacet = null;
+            DockerAncestorFingerprintFacet ancestorFacet = null;
             for (FingerprintFacet facet : facets) {
-                if (facet instanceof DockerDescendantFingerprintFacet) {
-                    descendantFacet = (DockerDescendantFingerprintFacet) facet;
+                if (facet instanceof DockerAncestorFingerprintFacet) {
+                    ancestorFacet = (DockerAncestorFingerprintFacet) facet;
                     break;
                 }
             }
             BulkChange bc = new BulkChange(f);
             try {
-                if (descendantFacet == null) {
-                    descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId);
-                    facets.add(descendantFacet);
+                if (ancestorFacet == null) {
+                    ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId);
+                    facets.add(ancestorFacet);
+                }
+                if (ancestorImageId != null) {
+                    ancestorFacet.addAncestorImageId(ancestorImageId);
                 }
-                descendantFacet.addDescendantImageId(descendantImageId);
-                descendantFacet.addFor(run);
-                DockerFingerprintAction.addToRun(f, ancestorImageId, run);
+                ancestorFacet.addFor(run);
+                DockerFingerprintAction.addToRun(f, descendantImageId, run);
                 bc.commit();
             } finally {
                 bc.abort();
             }
         }
-        Fingerprint f = forImage(run, descendantImageId);
-        Collection<FingerprintFacet> facets = f.getFacets();
-        DockerAncestorFingerprintFacet ancestorFacet = null;
-        for (FingerprintFacet facet : facets) {
-            if (facet instanceof DockerAncestorFingerprintFacet) {
-                ancestorFacet = (DockerAncestorFingerprintFacet) facet;
-                break;
-            }
-        }
-        BulkChange bc = new BulkChange(f);
-        try {
-            if (ancestorFacet == null) {
-                ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId);
-                facets.add(ancestorFacet);
-            }
-            if (ancestorImageId != null) {
-                ancestorFacet.addAncestorImageId(ancestorImageId);
-            }
-            ancestorFacet.addFor(run);
-            DockerFingerprintAction.addToRun(f, descendantImageId, run);
-            bc.commit();
-        } finally {
-            bc.abort();
-        }
     }
 
 }