Skip to content
Permalink
Browse files

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

…n build.

(cherry picked from commit d516702)
  • Loading branch information
jglick authored and olivergondza committed Mar 2, 2016
1 parent c69a88d commit 383a5aec51631412a10c4c8826448af057e606a6
@@ -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;
@@ -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 @@ public FakeMap make() {
}
};

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);
}

}

0 comments on commit 383a5ae

Please sign in to comment.
You can’t perform that action at this time.