Skip to content

Commit 1ff9c2a

Browse files
msohnmsohn
committed
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
1 parent 5db57fe commit 1ff9c2a

6 files changed

Lines changed: 227 additions & 1 deletion

File tree

‎Documentation/config-options.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,13 @@ Proxy configuration uses the standard Java mechanisms via class `java.net.ProxyS
134134
| `pack.window` | `10` | ✅ | Number of objects to try when looking for a delta base per thread searching for deltas. |
135135
| `pack.windowMemory` | `0` (unlimited) | ✅ | Maximum number of bytes to put into the delta search window. |
136136

137+
## reftable options
138+
139+
| option | default | git option | description |
140+
|---------|---------|------------|-------------|
141+
| `reftable.autoRefresh` | `false` | ⃞ | Whether to auto-refresh the reftable stack if it is out of date. |
142+
143+
137144
## __repack__ options
138145

139146
| option | default | git option | description |

‎org.eclipse.jgit.benchmarks/pom.xml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@
5252
<artifactId>org.eclipse.jgit.junit</artifactId>
5353
<version>${project.version}</version>
5454
</dependency>
55+
<dependency>
56+
<groupId>junit</groupId>
57+
<artifactId>junit</artifactId>
58+
</dependency>
5559
</dependencies>
5660

5761
<build>

‎org.eclipse.jgit.benchmarks/src/org/eclipse/jgit/benchmarks/GetRefsBenchmark.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import org.eclipse.jgit.api.Git;
2626
import org.eclipse.jgit.api.errors.GitAPIException;
27+
import org.eclipse.jgit.internal.storage.file.FileReftableDatabase;
2728
import org.eclipse.jgit.internal.storage.file.FileRepository;
2829
import org.eclipse.jgit.lib.BatchRefUpdate;
2930
import org.eclipse.jgit.lib.ConfigConstants;
@@ -39,8 +40,10 @@
3940
import org.eclipse.jgit.transport.ReceiveCommand;
4041
import org.eclipse.jgit.util.FS;
4142
import org.eclipse.jgit.util.FileUtils;
43+
import org.junit.Assume;
4244
import org.openjdk.jmh.annotations.Benchmark;
4345
import org.openjdk.jmh.annotations.BenchmarkMode;
46+
import org.openjdk.jmh.annotations.Fork;
4447
import org.openjdk.jmh.annotations.Measurement;
4548
import org.openjdk.jmh.annotations.Mode;
4649
import org.openjdk.jmh.annotations.OutputTimeUnit;
@@ -67,6 +70,9 @@ public static class BenchmarkState {
6770
@Param({ "true", "false" })
6871
boolean useRefTable;
6972

73+
@Param({ "true", "false" })
74+
boolean autoRefresh;
75+
7076
@Param({ "100", "1000", "10000", "100000" })
7177
int numBranches;
7278

@@ -82,6 +88,9 @@ public static class BenchmarkState {
8288
@Setup
8389
@SuppressWarnings("boxing")
8490
public void setupBenchmark() throws IOException, GitAPIException {
91+
// if we use RefDirectory skip autoRefresh = false
92+
Assume.assumeTrue(useRefTable || autoRefresh);
93+
8594
String firstBranch = "firstbranch";
8695
testDir = Files.createDirectory(Paths.get("testrepos"));
8796
String repoName = "branches-" + numBranches + "-trustStat-"
@@ -98,6 +107,9 @@ public void setupBenchmark() throws IOException, GitAPIException {
98107
((FileRepository) git.getRepository()).convertRefStorage(
99108
ConfigConstants.CONFIG_REF_STORAGE_REFTABLE, false,
100109
false);
110+
FileReftableDatabase refdb = (FileReftableDatabase) git
111+
.getRepository().getRefDatabase();
112+
refdb.setAutoRefresh(autoRefresh);
101113
} else {
102114
cfg.setEnum(ConfigConstants.CONFIG_CORE_SECTION, null,
103115
ConfigConstants.CONFIG_KEY_TRUST_STAT,
@@ -113,6 +125,7 @@ public void setupBenchmark() throws IOException, GitAPIException {
113125
System.out.println("Preparing test");
114126
System.out.println("- repository: \t\t" + repoPath);
115127
System.out.println("- refDatabase: \t\t" + refDatabaseType());
128+
System.out.println("- autoRefresh: \t\t" + autoRefresh);
116129
System.out.println("- trustStat: \t" + trustStat);
117130
System.out.println("- branches: \t\t" + numBranches);
118131

@@ -154,6 +167,7 @@ public void teardown() throws IOException {
154167
@OutputTimeUnit(TimeUnit.MICROSECONDS)
155168
@Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS)
156169
@Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS)
170+
@Fork(2)
157171
public void testGetExactRef(Blackhole blackhole, BenchmarkState state)
158172
throws IOException {
159173
String branchName = state.branches
@@ -166,6 +180,7 @@ public void testGetExactRef(Blackhole blackhole, BenchmarkState state)
166180
@OutputTimeUnit(TimeUnit.MICROSECONDS)
167181
@Warmup(iterations = 2, time = 100, timeUnit = TimeUnit.MILLISECONDS)
168182
@Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS)
183+
@Fork(2)
169184
public void testGetRefsByPrefix(Blackhole blackhole, BenchmarkState state)
170185
throws IOException {
171186
String branchPrefix = "refs/heads/branch/" + branchIndex.nextInt(100)

‎org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java‎

Lines changed: 133 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import static org.junit.Assert.assertSame;
2424
import static org.junit.Assert.assertTrue;
2525
import static org.junit.Assert.fail;
26+
import static org.junit.Assume.assumeTrue;
2627

2728
import java.io.File;
2829
import java.io.FileOutputStream;
@@ -33,8 +34,15 @@
3334
import java.util.Collection;
3435
import java.util.HashSet;
3536
import java.util.List;
36-
3737
import java.util.Set;
38+
import java.util.concurrent.Callable;
39+
import java.util.concurrent.CyclicBarrier;
40+
import java.util.concurrent.ExecutorService;
41+
import java.util.concurrent.Executors;
42+
import java.util.concurrent.Future;
43+
import java.util.concurrent.TimeUnit;
44+
45+
import org.eclipse.jgit.api.Git;
3846
import org.eclipse.jgit.lib.AnyObjectId;
3947
import org.eclipse.jgit.lib.Constants;
4048
import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -51,6 +59,10 @@
5159
import org.eclipse.jgit.revwalk.RevWalk;
5260
import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
5361
import org.eclipse.jgit.transport.ReceiveCommand;
62+
import org.eclipse.jgit.util.FS;
63+
import org.eclipse.jgit.util.FS.ExecutionResult;
64+
import org.eclipse.jgit.util.RawParseUtils;
65+
import org.eclipse.jgit.util.TemporaryBuffer;
5466
import org.junit.Test;
5567

5668
public class FileReftableTest extends SampleDataRepositoryTestCase {
@@ -64,6 +76,30 @@ public void setUp() throws Exception {
6476
db.convertToReftable(false, false);
6577
}
6678

79+
@SuppressWarnings("boxing")
80+
@Test
81+
public void testReloadIfNecessary() throws Exception {
82+
ObjectId id = db.resolve("master");
83+
try (FileRepository repo1 = new FileRepository(db.getDirectory());
84+
FileRepository repo2 = new FileRepository(db.getDirectory())) {
85+
((FileReftableDatabase) repo1.getRefDatabase())
86+
.setAutoRefresh(true);
87+
((FileReftableDatabase) repo2.getRefDatabase())
88+
.setAutoRefresh(true);
89+
FileRepository repos[] = { repo1, repo2 };
90+
for (int i = 0; i < 10; i++) {
91+
for (int j = 0; j < 2; j++) {
92+
FileRepository repo = repos[j];
93+
RefUpdate u = repo.getRefDatabase().newUpdate(
94+
String.format("branch%d", i * 10 + j), false);
95+
u.setNewObjectId(id);
96+
RefUpdate.Result r = u.update();
97+
assertEquals(Result.NEW, r);
98+
}
99+
}
100+
}
101+
}
102+
67103
@SuppressWarnings("boxing")
68104
@Test
69105
public void testRacyReload() throws Exception {
@@ -97,6 +133,54 @@ public void testRacyReload() throws Exception {
97133
}
98134
}
99135

136+
@Test
137+
public void testConcurrentRacyReload() throws Exception {
138+
ObjectId id = db.resolve("master");
139+
final CyclicBarrier barrier = new CyclicBarrier(2);
140+
141+
class UpdateRef implements Callable<RefUpdate.Result> {
142+
143+
private RefUpdate u;
144+
145+
UpdateRef(FileRepository repo, String branchName)
146+
throws IOException {
147+
u = repo.getRefDatabase().newUpdate(branchName,
148+
false);
149+
u.setNewObjectId(id);
150+
}
151+
152+
@Override
153+
public RefUpdate.Result call() throws Exception {
154+
barrier.await(); // wait for the other thread to prepare
155+
return u.update();
156+
}
157+
}
158+
159+
ExecutorService pool = Executors.newFixedThreadPool(2);
160+
try (FileRepository repo1 = new FileRepository(db.getDirectory());
161+
FileRepository repo2 = new FileRepository(db.getDirectory())) {
162+
((FileReftableDatabase) repo1.getRefDatabase())
163+
.setAutoRefresh(true);
164+
((FileReftableDatabase) repo2.getRefDatabase())
165+
.setAutoRefresh(true);
166+
for (int i = 0; i < 10; i++) {
167+
String branchName = String.format("branch%d",
168+
Integer.valueOf(i));
169+
Future<RefUpdate.Result> ru1 = pool
170+
.submit(new UpdateRef(repo1, branchName));
171+
Future<RefUpdate.Result> ru2 = pool
172+
.submit(new UpdateRef(repo2, branchName));
173+
assertTrue((ru1.get() == Result.NEW
174+
&& ru2.get() == Result.LOCK_FAILURE)
175+
|| (ru1.get() == Result.LOCK_FAILURE
176+
&& ru2.get() == Result.NEW));
177+
}
178+
} finally {
179+
pool.shutdown();
180+
pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
181+
}
182+
}
183+
100184
@Test
101185
public void testCompactFully() throws Exception {
102186
ObjectId c1 = db.resolve("master^^");
@@ -651,6 +735,54 @@ public void testGetRefsWithPrefixExcludingOverlappingPrefixes() throws IOExcepti
651735
checkContainsRef(refs, db.exactRef("HEAD"));
652736
}
653737

738+
@Test
739+
public void testExternalUpdate_bug_102() throws Exception {
740+
((FileReftableDatabase) db.getRefDatabase()).setAutoRefresh(true);
741+
assumeTrue(atLeastGitVersion(2, 45));
742+
Git git = Git.wrap(db);
743+
git.tag().setName("foo").call();
744+
Ref ref = db.exactRef("refs/tags/foo");
745+
assertNotNull(ref);
746+
runGitCommand("tag", "--force", "foo", "e");
747+
Ref e = db.exactRef("refs/heads/e");
748+
Ref foo = db.exactRef("refs/tags/foo");
749+
assertEquals(e.getObjectId(), foo.getObjectId());
750+
}
751+
752+
private String toString(TemporaryBuffer b) throws IOException {
753+
return RawParseUtils.decode(b.toByteArray());
754+
}
755+
756+
private ExecutionResult runGitCommand(String... args)
757+
throws IOException, InterruptedException {
758+
FS fs = db.getFS();
759+
ProcessBuilder pb = fs.runInShell("git", args);
760+
pb.directory(db.getWorkTree());
761+
System.err.println("PATH=" + pb.environment().get("PATH"));
762+
ExecutionResult result = fs.execute(pb, null);
763+
assertEquals(0, result.getRc());
764+
String err = toString(result.getStderr());
765+
if (!err.isEmpty()) {
766+
System.err.println(err);
767+
}
768+
String out = toString(result.getStdout());
769+
if (!out.isEmpty()) {
770+
System.out.println(out);
771+
}
772+
return result;
773+
}
774+
775+
private boolean atLeastGitVersion(int minMajor, int minMinor)
776+
throws IOException, InterruptedException {
777+
String version = toString(runGitCommand("version").getStdout())
778+
.split(" ")[2];
779+
System.out.println(version);
780+
String[] digits = version.split("\\.");
781+
int major = Integer.parseInt(digits[0]);
782+
int minor = Integer.parseInt(digits[1]);
783+
return (major >= minMajor) && (minor >= minMinor);
784+
}
785+
654786
private RefUpdate updateRef(String name) throws IOException {
655787
final RefUpdate ref = db.updateRef(name);
656788
ref.setNewObjectId(db.resolve(Constants.HEAD));

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import java.io.File;
1818
import java.io.IOException;
19+
import java.io.UncheckedIOException;
1920
import java.util.ArrayList;
2021
import java.util.Collections;
2122
import java.util.HashSet;
@@ -37,6 +38,7 @@
3738
import org.eclipse.jgit.internal.storage.reftable.ReftableDatabase;
3839
import org.eclipse.jgit.internal.storage.reftable.ReftableWriter;
3940
import org.eclipse.jgit.lib.BatchRefUpdate;
41+
import org.eclipse.jgit.lib.ConfigConstants;
4042
import org.eclipse.jgit.lib.Constants;
4143
import org.eclipse.jgit.lib.ObjectId;
4244
import org.eclipse.jgit.lib.ObjectIdRef;
@@ -70,13 +72,18 @@ public class FileReftableDatabase extends RefDatabase {
7072

7173
private final FileReftableStack reftableStack;
7274

75+
private boolean autoRefresh;
76+
7377
FileReftableDatabase(FileRepository repo) throws IOException {
7478
this(repo, new File(new File(repo.getCommonDirectory(), Constants.REFTABLE),
7579
Constants.TABLES_LIST));
7680
}
7781

7882
FileReftableDatabase(FileRepository repo, File refstackName) throws IOException {
7983
this.fileRepository = repo;
84+
this.autoRefresh = repo.getConfig().getBoolean(
85+
ConfigConstants.CONFIG_REFTABLE_SECTION,
86+
ConfigConstants.CONFIG_KEY_AUTOREFRESH, false);
8087
this.reftableStack = new FileReftableStack(refstackName,
8188
new File(fileRepository.getCommonDirectory(), Constants.REFTABLE),
8289
() -> fileRepository.fireEvent(new RefsChangedEvent()),
@@ -183,6 +190,7 @@ public RefUpdate newUpdate(String refName, boolean detach)
183190

184191
@Override
185192
public Ref exactRef(String name) throws IOException {
193+
autoRefresh();
186194
return reftableDatabase.exactRef(name);
187195
}
188196

@@ -193,6 +201,7 @@ public List<Ref> getRefs() throws IOException {
193201

194202
@Override
195203
public Map<String, Ref> getRefs(String prefix) throws IOException {
204+
autoRefresh();
196205
List<Ref> refs = reftableDatabase.getRefsByPrefix(prefix);
197206
RefList.Builder<Ref> builder = new RefList.Builder<>(refs.size());
198207
for (Ref r : refs) {
@@ -205,6 +214,7 @@ public Map<String, Ref> getRefs(String prefix) throws IOException {
205214
@Override
206215
public List<Ref> getRefsByPrefixWithExclusions(String include, Set<String> excludes)
207216
throws IOException {
217+
autoRefresh();
208218
return reftableDatabase.getRefsByPrefixWithExclusions(include, excludes);
209219
}
210220

@@ -223,6 +233,50 @@ public Ref peel(Ref ref) throws IOException {
223233

224234
}
225235

236+
/**
237+
* Whether to auto-refresh the reftable stack if it is out of date.
238+
*
239+
* @param autoRefresh
240+
* whether to auto-refresh the reftable stack if it is out of
241+
* date.
242+
*/
243+
public void setAutoRefresh(boolean autoRefresh) {
244+
this.autoRefresh = autoRefresh;
245+
}
246+
247+
/**
248+
* Whether the reftable stack is auto-refreshed if it is out of date.
249+
*
250+
* @return whether the reftable stack is auto-refreshed if it is out of
251+
* date.
252+
*/
253+
public boolean isAutoRefresh() {
254+
return autoRefresh;
255+
}
256+
257+
private void autoRefresh() {
258+
if (autoRefresh) {
259+
refresh();
260+
}
261+
}
262+
263+
/**
264+
* Check if the reftable stack is up to date, and if not, reload it.
265+
* <p>
266+
* {@inheritDoc}
267+
*/
268+
@Override
269+
public void refresh() {
270+
try {
271+
if (!reftableStack.isUpToDate()) {
272+
reftableDatabase.clearCache();
273+
reftableStack.reload();
274+
}
275+
} catch (IOException e) {
276+
throw new UncheckedIOException(e);
277+
}
278+
}
279+
226280
private Ref doPeel(Ref leaf) throws IOException {
227281
try (RevWalk rw = new RevWalk(fileRepository)) {
228282
RevObject obj = rw.parseAny(leaf.getObjectId());

0 commit comments

Comments
 (0)