Merge branch 'stable-7.0' into stable-7.1

* stable-7.0:
  Avoid reading packed-refs concurrently sometimes
  Improve flaky InterruptTimerTests

Change-Id: Icd62117b74eda95474e804ddb665820cf012ee80

Author: msohn

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

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.
@@ -977,8 +982,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 (trustPackedRefsStat) {
 		case NEVER:
 			break;
@@ -991,23 +1028,34 @@ PackedRefList getPackedRefs() throws IOException {
 			}
 			//$FALL-THROUGH$
 		case ALWAYS:
-			if (!curList.snapshot.isModified(packedRefsFile)) {
-				return curList;
+			if (!snapshot.isModified(packedRefsFile)) {
+				return false;
 			}
 			break;
 		case UNSET:
-			if (trustFolderStat
-					&& !curList.snapshot.isModified(packedRefsFile)) {
-				return curList;
+			if (trustFolderStat && !snapshot.isModified(packedRefsFile)) {
+				return false;
 			}
 			break;
 		}
-
-		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)
@@ -1031,7 +1079,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()));
 						}
@@ -1137,7 +1185,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) {
@@ -1491,21 +1539,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);
+			this.id = id;
+		}
+
+		public boolean shouldRefresh() throws IOException {
+			return true;
+		}
+	}
+
+	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;
-			id = i;
+		}
+
+		@Override
+		public boolean shouldRefresh() throws IOException {
+			return shouldRefreshPackedRefs(snapshot);
 		}
 	}
 
-	private static final PackedRefList NO_PACKED_REFS = new PackedRefList(
-			RefList.emptyList(), FileSnapshot.MISSING_FILE,
-			ObjectId.zeroId());
+	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) {