Merge branch 'stable-7.4' * stable-7.4: Avoid reading packed-refs concurrently sometimes Improve flaky InterruptTimerTests Change-Id: Ie01693564b9b13c98abaef4ba9864d888bf149a9
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/InterruptTimerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/InterruptTimerTest.java index 68de7d6..80419f7 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/InterruptTimerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/InterruptTimerTest.java
@@ -21,12 +21,12 @@ import org.junit.Test; public class InterruptTimerTest { - private static final int MULTIPLIER = 1; // Increase if tests get flaky + private static final int MULTIPLIER = 8; // Increase if tests get flaky private static final int BUFFER = 5; // Increase if tests get flaky private static final int REPEATS = 100; // Increase to stress test more - private static final int TOO_LONG = 3 * MULTIPLIER + BUFFER; - private static final int SHORT_ENOUGH = 1 * MULTIPLIER; + private static final int SHORT_ENOUGH = 1; + private static final int TOO_LONG = SHORT_ENOUGH * MULTIPLIER + BUFFER; private static final int TIMEOUT_LONG_ENOUGH = TOO_LONG; private static final int TIMEOUT_TOO_SHORT = SHORT_ENOUGH; @@ -53,6 +53,7 @@ public void testShortEnough() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Not Interrupted", interrupted, 0); } @@ -66,6 +67,7 @@ public void testTooLong() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Interrupted", interrupted, 1); } @@ -80,6 +82,7 @@ public void testNotInterruptedAfterEnd() { Thread.sleep(TIMEOUT_LONG_ENOUGH * 3); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Not Interrupted Even After End", interrupted, 0); } @@ -96,6 +99,7 @@ public void testRestartBeforeTimeout() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Not Interrupted Even When Restarted Before Timeout", interrupted, 0); } @@ -112,6 +116,7 @@ public void testSecondExpiresBeforeFirst() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Interrupted Even When Second Timeout Expired Before First", interrupted, 1); } @@ -126,6 +131,7 @@ public void testRepeatedShortEnough() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } } assertEquals("Was Never Interrupted", interrupted, 0); @@ -140,8 +146,8 @@ public void testRepeatedTooLong() { Thread.sleep(TOO_LONG); timer.end(); } catch (InterruptedException e) { - Thread.currentThread().interrupt(); interrupted++; + Thread.currentThread().interrupt(); } } assertEquals("Was always Interrupted", interrupted, REPEATS); @@ -157,6 +163,7 @@ public void testRepeatedShortThanTooLong() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } } assertEquals("Was Not Interrupted Early", interrupted, 0); @@ -166,6 +173,7 @@ public void testRepeatedShortThanTooLong() { timer.end(); } catch (InterruptedException e) { interrupted++; + Thread.currentThread().interrupt(); } assertEquals("Was Interrupted On Long", interrupted, 1); }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index 319a9ed..a439f43 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -49,6 +49,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantLock; @@ -150,6 +152,9 @@ public class RefDirectory extends RefDatabase { /** Immutable sorted list of packed references. */ final AtomicReference<PackedRefList> packedRefs = new AtomicReference<>(); + private final AtomicReference<PackedRefsRefresher> packedRefsRefresher = + new AtomicReference<>(); + /** * Lock for coordinating operations within a single process that may contend * on the {@code packed-refs} file. @@ -966,8 +971,40 @@ else if (0 <= (idx = packed.find(dst.getName()))) } PackedRefList getPackedRefs() throws IOException { - final PackedRefList curList = packedRefs.get(); + PackedRefList curList = packedRefs.get(); + if (!curList.shouldRefresh()) { + return curList; + } + return getPackedRefsRefresher(curList).getOrThrowIOException(); + } + private PackedRefsRefresher getPackedRefsRefresher(PackedRefList curList) + throws IOException { + PackedRefsRefresher refresher = packedRefsRefresher.get(); + if (refresher != null && !refresher.shouldRefresh()) { + return refresher; + } + // This synchronized is NOT needed for correctness. Instead it is used + // as a throttling mechanism to ensure that only one "read" thread does + // the work to refresh the file. In order to avoid stalling writes which + // must already be serialized and tend to be a bottleneck, + // the refreshPackedRefs() need not be synchronized. + synchronized (this) { + if (packedRefsRefresher.get() != refresher) { + refresher = packedRefsRefresher.get(); + if (refresher != null) { + // Refresher now guaranteed to have been created after the + // current thread entered getPackedRefsRefresher(), even if + // it's currently out of date. + return refresher; + } + } + refresher = createPackedRefsRefresherAsLatest(curList); + } + return runAndClear(refresher); + } + + private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOException { switch (coreConfig.getTrustPackedRefsStat()) { case NEVER: break; @@ -980,19 +1017,31 @@ PackedRefList getPackedRefs() throws IOException { } //$FALL-THROUGH$ case ALWAYS: - if (!curList.snapshot.isModified(packedRefsFile)) { - return curList; + if (!snapshot.isModified(packedRefsFile)) { + return false; } break; case INHERIT: // only used in CoreConfig internally } - - return refreshPackedRefs(curList); + return true; } PackedRefList refreshPackedRefs() throws IOException { - return refreshPackedRefs(packedRefs.get()); + return runAndClear(createPackedRefsRefresherAsLatest(packedRefs.get())) + .getOrThrowIOException(); + } + + private PackedRefsRefresher createPackedRefsRefresherAsLatest(PackedRefList curList) { + PackedRefsRefresher refresher = new PackedRefsRefresher(curList); + packedRefsRefresher.set(refresher); + return refresher; + } + + private PackedRefsRefresher runAndClear(PackedRefsRefresher refresher) { + refresher.run(); + packedRefsRefresher.compareAndSet(refresher, null); + return refresher; } private PackedRefList refreshPackedRefs(PackedRefList curList) @@ -1016,7 +1065,7 @@ private PackedRefList readPackedRefs() throws IOException { new DigestInputStream( new FileInputStream(f), digest), UTF_8))) { - return new PackedRefList(parsePackedRefs(br), + return new NonEmptyPackedRefList(parsePackedRefs(br), snapshot, ObjectId.fromRaw(digest.digest())); } @@ -1122,7 +1171,7 @@ protected void writeFile(String name, byte[] content) throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name)); byte[] digest = Constants.newMessageDigest().digest(content); - PackedRefList newPackedList = new PackedRefList( + PackedRefList newPackedList = new NonEmptyPackedRefList( refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest)); packedRefs.compareAndSet(oldPackedList, newPackedList); if (changed) { @@ -1476,21 +1525,57 @@ static void sleep(long ms) throws InterruptedIOException { } static class PackedRefList extends RefList<Ref> { - - private final FileSnapshot snapshot; - private final ObjectId id; - private PackedRefList(RefList<Ref> src, FileSnapshot s, ObjectId i) { + PackedRefList() { + this(RefList.emptyList(), ObjectId.zeroId()); + } + + protected PackedRefList(RefList<Ref> src, ObjectId id) { super(src); - snapshot = s; - id = i; + this.id = id; + } + + public boolean shouldRefresh() throws IOException { + return true; } } - private static final PackedRefList NO_PACKED_REFS = new PackedRefList( - RefList.emptyList(), FileSnapshot.MISSING_FILE, - ObjectId.zeroId()); + private static final PackedRefList NO_PACKED_REFS = new PackedRefList(); + + private class NonEmptyPackedRefList extends PackedRefList { + private final FileSnapshot snapshot; + + private NonEmptyPackedRefList(RefList<Ref> src, FileSnapshot s, ObjectId id) { + super(src, id); + snapshot = s; + } + + @Override + public boolean shouldRefresh() throws IOException { + return shouldRefreshPackedRefs(snapshot); + } + } + + private class PackedRefsRefresher extends FutureTask<PackedRefList> { + private final FileSnapshot snapshot = FileSnapshot.save(packedRefsFile); + + public PackedRefsRefresher(PackedRefList curList) { + super(() -> refreshPackedRefs(curList)); + } + + public PackedRefList getOrThrowIOException() throws IOException { + try { + return get(); + } catch (ExecutionException | InterruptedException e) { + throw new IOException(e); + } + } + + public boolean shouldRefresh() throws IOException { + return shouldRefreshPackedRefs(snapshot); + } + } private static LooseSymbolicRef newSymbolicRef(FileSnapshot snapshot, String name, String target) {