[FIXED JENKINS-22767] Make sure only one thread actually loads a give…

…n build. (cherry picked from commit d516702)
jenkinsci · Mar 2, 2016 · 383a5ae · 383a5ae
1 parent c69a88d
commit 383a5ae
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 3 deletions.
diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
@@ -353,7 +353,19 @@ public R getByNumber(int n) {
             if (v!=null)        return v;       // already in memory
             // otherwise fall through to load
         }
-        return load(n, null);
+        synchronized (this) {
+            if (index.byNumber.containsKey(n)) { // JENKINS-22767: recheck inside lock
+                BuildReference<R> ref = index.byNumber.get(n);
+                if (ref == null) {
+                    return null;
+                }
+                R v = unwrap(ref);
+                if (v != null) {
+                    return v;
+                }
+            }
+            return load(n, null);
+        }
     }
 
     protected final synchronized void proposeNewNumber(int number) throws IllegalStateException {
@@ -443,7 +455,8 @@ private Index copy() {
      * 
      * @return null if the data failed to load.
      */
-    protected R load(int n, Index editInPlace) {
+    private R load(int n, Index editInPlace) {
+        assert Thread.holdsLock(this);
         assert dir != null;
         R v = load(new File(dir, String.valueOf(n)), editInPlace);
         if (v==null && editInPlace!=null) {
@@ -460,7 +473,8 @@ protected R load(int n, Index editInPlace) {
      *      If non-null, update this data structure.
      *      Otherwise do a copy-on-write of {@link #index}
      */
-    protected synchronized R load(File dataDir, Index editInPlace) {
+    private R load(File dataDir, Index editInPlace) {
+        assert Thread.holdsLock(this);
         try {
             R r = retrieve(dataDir);
             if (r==null)    return null;

diff --git a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
@@ -23,6 +23,7 @@
  */
 package jenkins.model.lazy;
 
+import java.io.File;
 import static org.junit.Assert.*;
 
 import jenkins.model.lazy.AbstractLazyLoadRunMap.Direction;
@@ -31,13 +32,19 @@
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.SortedMap;
+import java.util.concurrent.Callable;
+import java.util.concurrent.Future;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Level;
+import jenkins.util.Timer;
 import org.junit.BeforeClass;
 import org.jvnet.hudson.test.Issue;
 
@@ -72,6 +79,29 @@ protected BuildReference<Build> createReference(Build r) {
         }
     };
 
+    private final Map<Integer,Semaphore> slowBuilderStartSemaphores = new HashMap<>();
+    private final Map<Integer,Semaphore> slowBuilderEndSemaphores = new HashMap<>();
+    private final Map<Integer,AtomicInteger> slowBuilderLoadCount = new HashMap<>();
+    @Rule
+    public FakeMapBuilder slowBuilder = new FakeMapBuilder() {
+        @Override
+        public FakeMap make() {
+            return new FakeMap(getDir()) {
+                @Override
+                protected Build retrieve(File dir) throws IOException {
+                    Build b = super.retrieve(dir);
+                    slowBuilderStartSemaphores.get(b.n).release();
+                    try {
+                        slowBuilderEndSemaphores.get(b.n).acquire();
+                    } catch (InterruptedException x) {
+                        throw new IOException(x);
+                    }
+                    slowBuilderLoadCount.get(b.n).incrementAndGet();
+                    return b;
+                }
+            };
+        }
+    };
 
     @BeforeClass
     public static void setUpClass() {
@@ -358,4 +388,36 @@ public void entrySetContains() {
             assertTrue(a.entrySet().contains(e));
         }
     }
+
+    @Issue("JENKINS-22767")
+    @Test
+    public void slowRetrieve() throws Exception {
+        for (int i = 1; i <= 3; i++) {
+            slowBuilder.add(i);
+            slowBuilderStartSemaphores.put(i, new Semaphore(0));
+            slowBuilderEndSemaphores.put(i, new Semaphore(0));
+            slowBuilderLoadCount.put(i, new AtomicInteger());
+        }
+        final FakeMap m = slowBuilder.make();
+        Future<Build> firstLoad = Timer.get().submit(new Callable<Build>() {
+            @Override
+            public Build call() throws Exception {
+                return m.getByNumber(2);
+            }
+        });
+        Future<Build> secondLoad = Timer.get().submit(new Callable<Build>() {
+            @Override
+            public Build call() throws Exception {
+                return m.getByNumber(2);
+            }
+        });
+        slowBuilderStartSemaphores.get(2).acquire(1);
+        // now one of them is inside retrieve(…); the other is waiting for the lock
+        slowBuilderEndSemaphores.get(2).release(2); // allow both to proceed
+        Build first = firstLoad.get();
+        Build second = secondLoad.get();
+        assertEquals(1, slowBuilderLoadCount.get(2).get());
+        assertSame(second, first);
+    }
+
 }