From 383a5aec51631412a10c4c8826448af057e606a6 Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Mon, 18 Jan 2016 21:47:19 -0500
Subject: [PATCH] [FIXED JENKINS-22767] Make sure only one thread actually
 loads a given build. (cherry picked from commit
 d5167025a204750633c931ea8c1fff8d7561ab9c)

---
 .../model/lazy/AbstractLazyLoadRunMap.java    | 20 +++++-
 .../lazy/AbstractLazyLoadRunMapTest.java      | 62 +++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
index c93b4e8be1ce..25ac5f4dfcb3 100644
--- 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
index 88ca41c5d18a..4167f668a734 100644
--- 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);
+    }
+
 }