[JENKINS-41880] Synchronise on Fingerprint

[bjoernhaeuser]

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?

[bjoernhaeuser]

Thanks!

[jglick]

Please revert formatting-only changes, and configure your editor to not change whitespace in lines you have not otherwise edited.

[jglick]

Unreadable diff. This is much easier to follow, but generally I recommend not changing indentation of large code blocks, since it messes up other things like cherry-picks and merges.

[jglick]

Unrelated, revert.

[jglick]

The PR could be reduced to the addition of this line and its closing brace.

Anyway, this looks wrong to me. A Fingerprint, by design, represents something that is (potentially) shared across multiple builds, typically of different jobs. And Fingerprint.save synchronizes on this. So I suspect you rather want to be synchronizing on each Fingerprint being manipulated. Try:

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
index 047b4e4..95c62f4 100644
--- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java
+++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java
@@ -257,6 +257,7 @@ public class DockerFingerprints {
         long timestamp = System.currentTimeMillis();
         if (ancestorImageId != null) {
             Fingerprint f = forImage(run, ancestorImageId);
+            synchronized (f) {
             Collection<FingerprintFacet> facets = f.getFacets();
             DockerDescendantFingerprintFacet descendantFacet = null;
             for (FingerprintFacet facet : facets) {
@@ -278,8 +279,10 @@ public class DockerFingerprints {
             } finally {
                 bc.abort();
             }
+            }
         }
         Fingerprint f = forImage(run, descendantImageId);
+        synchronized (f) {
         Collection<FingerprintFacet> facets = f.getFacets();
         DockerAncestorFingerprintFacet ancestorFacet = null;
         for (FingerprintFacet facet : facets) {
@@ -303,6 +306,7 @@ public class DockerFingerprints {
         } finally {
             bc.abort();
         }
+        }
     }
 
 }

and see if that fixes your issue.

[bjoernhaeuser]

I removed all unrelated changes and went with the suggestion to synchronize the fingerprint. I just tested this on our jenkins installation and it looks very promising.

[bjoernhaeuser]

promising as in: no ConcurrentModificationExceptions anymore

[bjoernhaeuser]

@jglick could you take another look please? :)

[crummy]

Looks like some indentation is needed here, and after the next synchronized block too

[jglick]

The idea was to avoid touching every line in between, which makes diffs harder to read and complicates merges & cherry-picks.

[bjoernhaeuser]

This is now runnig for two weeks in our production setup and the error disappeared.

[bjoernhaeuser]

@jglick Could you lift your "Requested changes"?

[oleg-nenashev]

LGTM, at least it should reduce the risk of race conditions significantly eve if there issues in lazy loading for fingerprints.

[jglick]

The idea was to avoid touching every line in between, which makes diffs harder to read and complicates merges & cherry-picks.

[jglick]

@abayer I think you own this

[bjoernhaeuser]

Thanks everyone!

[abayer]

@jglick - I don't think so - I've literally never touched this plugin, don't have permissions on it, and am traveling/on vacation for the next two weeks. =)