FileReftableDatabase: consider ref updates by another process

FileReftableDatabase didn't consider that refs might be changed by
another process e.g. using git (which started supporting reftable with
version 2.45).

Add a test creating a light-weight tag which is updated using git
running in another process and assert that FileReftableDatabase
recognizes the tag modification.

FileReftableStack#addReftable checks if the stack is up-to-date while it
holds the FileLock for tables.list, if it is not up-to-date the
RefUpdate fails with a LOCK_FAILURE to protect against lost ref updates
if another instance of FileReftableDatabase or another thread or process
updated the reftable stack since we last read it.

If option `reftable.autoRefresh = true` or `setAutoRefresh(true)` was
called check before each ref resolution if the reftable stack is
up-to-date and, if necessary, reload the reftable stack automatically.
Calling `setAutoRefresh(true)` takes precedence over the configured
value for option `reftable.autoRefresh`.

Add a testConcurrentRacyReload which tests that updates still abort ref
updates if the reftable stack the update is based on was outdated.

Bug: jgit-102
Change-Id: I1f9faa2afdbfff27e83ff295aef6d572babed4fe

6 files changed, 227 insertions, 1 deletions

diff --git a/Documentation/config-options.md b/Documentation/config-options.md
index 807d4a8002..1dfe95f850 100644
--- a/Documentation/config-options.md
+++ b/Documentation/config-options.md
@@ -134,6 +134,13 @@ Proxy configuration uses the standard Java mechanisms via class `java.net.ProxyS
 | `pack.window` | `10` | ✅ | Number of objects to try when looking for a delta base per thread searching for deltas. |
 | `pack.windowMemory` | `0` (unlimited) | ✅ | Maximum number of bytes to put into the delta search window. |
 
+## reftable options
+
+|  option | default | git option | description |
+|---------|---------|------------|-------------|
+| `reftable.autoRefresh` | `false` | ⃞ | Whether to auto-refresh the reftable stack if it is out of date. |
+
+
 ## __repack__ options
 
 |  option | default | git option | description |
diff --git a/org.eclipse.jgit.benchmarks/pom.xml b/org.eclipse.jgit.benchmarks/pom.xml
index c92fa9f156..e531245971 100644
--- a/org.eclipse.jgit.benchmarks/pom.xml
+++ b/org.eclipse.jgit.benchmarks/pom.xml
@@ -52,6 +52,10 @@
       <artifactId>org.eclipse.jgit.junit</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>
+    </dependency>
   </dependencies>
 
   <build>
diff --git a/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java b/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java
index ea279fbba9..44e862e7c8 100644
--- a/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java
+++ b/org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java
@@ -24,6 +24,7 @@ import java.util.stream.IntStream;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.internal.storage.file.FileReftableDatabase;
 import org.eclipse.jgit.internal.storage.file.FileRepository;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.ConfigConstants;
@@ -39,8 +40,10 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.FileUtils;
+import org.junit.Assume;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
 import org.openjdk.jmh.annotations.Measurement;
 import org.openjdk.jmh.annotations.Mode;
 import org.openjdk.jmh.annotations.OutputTimeUnit;
@@ -67,6 +70,9 @@ public class GetRefsBenchmark {
 		@Param({ "true", "false" })
 		boolean useRefTable;
 
+		@Param({ "true", "false" })
+		boolean autoRefresh;
+
 		@Param({ "100", "1000", "10000", "100000" })
 		int numBranches;
 
@@ -82,6 +88,9 @@ public class GetRefsBenchmark {
 		@Setup
 		@SuppressWarnings("boxing")
 		public void setupBenchmark() throws IOException, GitAPIException {
+			// if we use RefDirectory skip autoRefresh = false
+			Assume.assumeTrue(useRefTable || autoRefresh);
+
 			String firstBranch = "firstbranch";
 			testDir = Files.createDirectory(Paths.get("testrepos"));
 			String repoName = "branches-" + numBranches + "-trustStat-"
@@ -98,6 +107,9 @@ public class GetRefsBenchmark {
 				((FileRepository) git.getRepository()).convertRefStorage(
 						ConfigConstants.CONFIG_REF_STORAGE_REFTABLE, false,
 						false);
+				FileReftableDatabase refdb = (FileReftableDatabase) git
+						.getRepository().getRefDatabase();
+				refdb.setAutoRefresh(autoRefresh);
 			} else {
 				cfg.setEnum(ConfigConstants.CONFIG_CORE_SECTION, null,
 						ConfigConstants.CONFIG_KEY_TRUST_STAT,
@@ -113,6 +125,7 @@ public class GetRefsBenchmark {
 			System.out.println("Preparing test");
 			System.out.println("- repository: \t\t" + repoPath);
 			System.out.println("- refDatabase: \t\t" + refDatabaseType());
+			System.out.println("- autoRefresh: \t\t" + autoRefresh);
 			System.out.println("- trustStat: \t" + trustStat);
 			System.out.println("- branches: \t\t" + numBranches);
 
@@ -154,6 +167,7 @@ public class GetRefsBenchmark {
 	@OutputTimeUnit(TimeUnit.MICROSECONDS)
 	@Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS)
 	@Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS)
+	@Fork(2)
 	public void testGetExactRef(Blackhole blackhole, BenchmarkState state)
 			throws IOException {
 		String branchName = state.branches
@@ -166,6 +180,7 @@ public class GetRefsBenchmark {
 	@OutputTimeUnit(TimeUnit.MICROSECONDS)
 	@Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS)
 	@Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS)
+	@Fork(2)
 	public void testGetRefsByPrefix(Blackhole blackhole, BenchmarkState state)
 			throws IOException {
 		String branchPrefix = "refs/heads/branch/" + branchIndex.nextInt(100)
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java
index 758ee93c30..5756b41442 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java
@@ -23,6 +23,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
 
 import java.io.File;
 import java.io.FileOutputStream;
@@ -33,8 +34,15 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
-
 import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -51,6 +59,10 @@ import org.eclipse.jgit.lib.RepositoryCache;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
 import org.eclipse.jgit.transport.ReceiveCommand;
+import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FS.ExecutionResult;
+import org.eclipse.jgit.util.RawParseUtils;
+import org.eclipse.jgit.util.TemporaryBuffer;
 import org.junit.Test;
 
 public class FileReftableTest extends SampleDataRepositoryTestCase {
@@ -66,6 +78,30 @@ public class FileReftableTest extends SampleDataRepositoryTestCase {
 
 	@SuppressWarnings("boxing")
 	@Test
+	public void testReloadIfNecessary() throws Exception {
+		ObjectId id = db.resolve("master");
+		try (FileRepository repo1 = new FileRepository(db.getDirectory());
+				FileRepository repo2 = new FileRepository(db.getDirectory())) {
+			((FileReftableDatabase) repo1.getRefDatabase())
+					.setAutoRefresh(true);
+			((FileReftableDatabase) repo2.getRefDatabase())
+					.setAutoRefresh(true);
+			FileRepository repos[] = { repo1, repo2 };
+			for (int i = 0; i < 10; i++) {
+				for (int j = 0; j < 2; j++) {
+					FileRepository repo = repos[j];
+					RefUpdate u = repo.getRefDatabase().newUpdate(
+							String.format("branch%d", i * 10 + j), false);
+					u.setNewObjectId(id);
+					RefUpdate.Result r = u.update();
+					assertEquals(Result.NEW, r);
+				}
+			}
+		}
+	}
+
+	@SuppressWarnings("boxing")
+	@Test
 	public void testRacyReload() throws Exception {
 		ObjectId id = db.resolve("master");
 		int retry = 0;
@@ -98,6 +134,54 @@ public class FileReftableTest extends SampleDataRepositoryTestCase {
 	}
 
 	@Test
+	public void testConcurrentRacyReload() throws Exception {
+		ObjectId id = db.resolve("master");
+		final CyclicBarrier barrier = new CyclicBarrier(2);
+
+		class UpdateRef implements Callable<RefUpdate.Result> {
+
+			private RefUpdate u;
+
+			UpdateRef(FileRepository repo, String branchName)
+					throws IOException {
+				u = repo.getRefDatabase().newUpdate(branchName,
+						false);
+				u.setNewObjectId(id);
+			}
+
+			@Override
+			public RefUpdate.Result call() throws Exception {
+				barrier.await(); // wait for the other thread to prepare
+				return u.update();
+			}
+		}
+
+		ExecutorService pool = Executors.newFixedThreadPool(2);
+		try (FileRepository repo1 = new FileRepository(db.getDirectory());
+				FileRepository repo2 = new FileRepository(db.getDirectory())) {
+			((FileReftableDatabase) repo1.getRefDatabase())
+					.setAutoRefresh(true);
+			((FileReftableDatabase) repo2.getRefDatabase())
+					.setAutoRefresh(true);
+			for (int i = 0; i < 10; i++) {
+				String branchName = String.format("branch%d",
+						Integer.valueOf(i));
+				Future<RefUpdate.Result> ru1 = pool
+						.submit(new UpdateRef(repo1, branchName));
+				Future<RefUpdate.Result> ru2 = pool
+						.submit(new UpdateRef(repo2, branchName));
+				assertTrue((ru1.get() == Result.NEW
+						&& ru2.get() == Result.LOCK_FAILURE)
+						|| (ru1.get() == Result.LOCK_FAILURE
+								&& ru2.get() == Result.NEW));
+			}
+		} finally {
+			pool.shutdown();
+			pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
+		}
+	}
+
+	@Test
 	public void testCompactFully() throws Exception {
 		ObjectId c1 = db.resolve("master^^");
 		ObjectId c2 = db.resolve("master^");
@@ -651,6 +735,54 @@ public class FileReftableTest extends SampleDataRepositoryTestCase {
 		checkContainsRef(refs, db.exactRef("HEAD"));
 	}
 
+	@Test
+	public void testExternalUpdate_bug_102() throws Exception {
+		((FileReftableDatabase) db.getRefDatabase()).setAutoRefresh(true);
+		assumeTrue(atLeastGitVersion(2, 45));
+		Git git = Git.wrap(db);
+		git.tag().setName("foo").call();
+		Ref ref = db.exactRef("refs/tags/foo");
+		assertNotNull(ref);
+		runGitCommand("tag", "--force", "foo", "e");
+		Ref e = db.exactRef("refs/heads/e");
+		Ref foo = db.exactRef("refs/tags/foo");
+		assertEquals(e.getObjectId(), foo.getObjectId());
+	}
+
+	private String toString(TemporaryBuffer b) throws IOException {
+		return RawParseUtils.decode(b.toByteArray());
+	}
+
+	private ExecutionResult runGitCommand(String... args)
+			throws IOException, InterruptedException {
+		FS fs = db.getFS();
+		ProcessBuilder pb = fs.runInShell("git", args);
+		pb.directory(db.getWorkTree());
+		System.err.println("PATH=" + pb.environment().get("PATH"));
+		ExecutionResult result = fs.execute(pb, null);
+		assertEquals(0, result.getRc());
+		String err = toString(result.getStderr());
+		if (!err.isEmpty()) {
+			System.err.println(err);
+		}
+		String out = toString(result.getStdout());
+		if (!out.isEmpty()) {
+			System.out.println(out);
+		}
+		return result;
+	}
+
+	private boolean atLeastGitVersion(int minMajor, int minMinor)
+			throws IOException, InterruptedException {
+		String version = toString(runGitCommand("version").getStdout())
+				.split(" ")[2];
+		System.out.println(version);
+		String[] digits = version.split("\\.");
+		int major = Integer.parseInt(digits[0]);
+		int minor = Integer.parseInt(digits[1]);
+		return (major >= minMajor) && (minor >= minMinor);
+	}
+
 	private RefUpdate updateRef(String name) throws IOException {
 		final RefUpdate ref = db.updateRef(name);
 		ref.setNewObjectId(db.resolve(Constants.HEAD));
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java
index 6040ad554d..d70fa839e9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java
@@ -16,6 +16,7 @@ import static org.eclipse.jgit.lib.Ref.Storage.PACKED;
 
 import java.io.File;
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -37,6 +38,7 @@ import org.eclipse.jgit.internal.storage.reftable.ReftableBatchRefUpdate;
 import org.eclipse.jgit.internal.storage.reftable.ReftableDatabase;
 import org.eclipse.jgit.internal.storage.reftable.ReftableWriter;
 import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectIdRef;
@@ -70,6 +72,8 @@ public class FileReftableDatabase extends RefDatabase {
 
 	private final FileReftableStack reftableStack;
 
+	private boolean autoRefresh;
+
 	FileReftableDatabase(FileRepository repo) throws IOException {
 		this(repo, new File(new File(repo.getCommonDirectory(), Constants.REFTABLE),
 				Constants.TABLES_LIST));
@@ -77,6 +81,9 @@ public class FileReftableDatabase extends RefDatabase {
 
 	FileReftableDatabase(FileRepository repo, File refstackName) throws IOException {
 		this.fileRepository = repo;
+		this.autoRefresh = repo.getConfig().getBoolean(
+				ConfigConstants.CONFIG_REFTABLE_SECTION,
+				ConfigConstants.CONFIG_KEY_AUTOREFRESH, false);
 		this.reftableStack = new FileReftableStack(refstackName,
 				new File(fileRepository.getCommonDirectory(), Constants.REFTABLE),
 			() -> fileRepository.fireEvent(new RefsChangedEvent()),
@@ -183,6 +190,7 @@ public class FileReftableDatabase extends RefDatabase {
 
 	@Override
 	public Ref exactRef(String name) throws IOException {
+		autoRefresh();
 		return reftableDatabase.exactRef(name);
 	}
 
@@ -193,6 +201,7 @@ public class FileReftableDatabase extends RefDatabase {
 
 	@Override
 	public Map<String, Ref> getRefs(String prefix) throws IOException {
+		autoRefresh();
 		List<Ref> refs = reftableDatabase.getRefsByPrefix(prefix);
 		RefList.Builder<Ref> builder = new RefList.Builder<>(refs.size());
 		for (Ref r : refs) {
@@ -205,6 +214,7 @@ public class FileReftableDatabase extends RefDatabase {
 	@Override
 	public List<Ref> getRefsByPrefixWithExclusions(String include, Set<String> excludes)
 			throws IOException {
+		autoRefresh();
 		return reftableDatabase.getRefsByPrefixWithExclusions(include, excludes);
 	}
 
@@ -223,6 +233,50 @@ public class FileReftableDatabase extends RefDatabase {
 
 	}
 
+	/**
+	 * Whether to auto-refresh the reftable stack if it is out of date.
+	 *
+	 * @param autoRefresh
+	 *            whether to auto-refresh the reftable stack if it is out of
+	 *            date.
+	 */
+	public void setAutoRefresh(boolean autoRefresh) {
+		this.autoRefresh = autoRefresh;
+	}
+
+	/**
+	 * Whether the reftable stack is auto-refreshed if it is out of date.
+	 *
+	 * @return whether the reftable stack is auto-refreshed if it is out of
+	 *         date.
+	 */
+	public boolean isAutoRefresh() {
+		return autoRefresh;
+	}
+
+	private void autoRefresh() {
+		if (autoRefresh) {
+			refresh();
+		}
+	}
+
+	/**
+	 * Check if the reftable stack is up to date, and if not, reload it.
+	 * <p>
+	 * {@inheritDoc}
+	 */
+	@Override
+	public void refresh() {
+		try {
+			if (!reftableStack.isUpToDate()) {
+				reftableDatabase.clearCache();
+				reftableStack.reload();
+			}
+		} catch (IOException e) {
+			throw new UncheckedIOException(e);
+		}
+	}
+
 	private Ref doPeel(Ref leaf) throws IOException {
 		try (RevWalk rw = new RevWalk(fileRepository)) {
 			RevObject obj = rw.parseAny(leaf.getObjectId());
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
index 30e7de47c3..f62bf161a7 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
@@ -1093,4 +1093,18 @@ public final class ConfigConstants {
 	 * @since 7.1
 	 */
 	public static final String CONFIG_KEY_LOAD_REV_INDEX_IN_PARALLEL = "loadRevIndexInParallel";
+
+	/**
+	 * The "reftable" section
+	 *
+	 * @since 7.2
+	 */
+	public static final String CONFIG_REFTABLE_SECTION = "reftable";
+
+	/**
+	 * The "autorefresh" key
+	 *
+	 * @since 7.2
+	 */
+	public static final String CONFIG_KEY_AUTOREFRESH = "autorefresh";
 }