From 9c33f7364d41956240818ba12d8b79d5ea846162 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Jul 2017 14:19:36 -0400 Subject: [PATCH] RefDirectory: Throw exception if CAS of packed ref list fails The contents of the packedRefList AtomicReference should never differ from what we expect prior to writing, because this segment of the code is protected by the packed-refs lock file on disk. If it does happen, whether due to programmer error or a rogue process not respecting the locking protocol, it's better to let the caller know than to silently drop the whole commit operation on the floor. The existing concurrentOnlyOneWritesPackedRefs test is inherently nondeterministic as written, and was already about 6% flaky as measured by bazel: $ bazel test --runs_per_test=200 //org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_GcPackRefsTest ... INFO: Elapsed time: 42.608s, Critical Path: 10.35s //org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_GcPackRefsTest FAILED in 12 out of 200 in 1.6s Stats over 200 runs: max = 1.6s, min = 1.1s, avg = 1.3s, dev = 0.1s This flakiness was caused by the assumption that exactly one of the 2 threads would fail, when both might actually succeed in practice due to racing on the compare-and-swap. For whatever reason, this change affected the interleaving behavior in such a way that the flakiness jumped to around 50%. Making the interleaving of the test fully deterministic is beyond the scope of this change, but a simple tweak to the assertion is enough to make it pass consistently 200+ times both before and after this change. Change-Id: I5ff4dc39ee05bda88d47909acb70118f3d0c8f74 --- .../internal/storage/file/GcPackRefsTest.java | 28 +++++++++---------- .../internal/storage/file/RefDirectory.java | 20 +++++++++++-- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java index 11a2a22ed..c43bdbd29 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java @@ -43,12 +43,13 @@ package org.eclipse.jgit.internal.storage.file; -import static java.lang.Integer.valueOf; +import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; import java.io.File; import java.io.IOException; @@ -74,6 +75,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig; import org.junit.Test; +@SuppressWarnings("boxing") public class GcPackRefsTest extends GcTestCase { @Test public void looseRefPacked() throws Exception { @@ -100,27 +102,23 @@ public void concurrentOnlyOneWritesPackedRefs() throws Exception { RevBlob a = tr.blob("a"); tr.lightweightTag("t", a); - final CyclicBarrier syncPoint = new CyclicBarrier(2); + CyclicBarrier syncPoint = new CyclicBarrier(2); - Callable packRefs = new Callable() { - - /** @return 0 for success, 1 in case of error when writing pack */ - @Override - public Integer call() throws Exception { - syncPoint.await(); - try { - gc.packRefs(); - return valueOf(0); - } catch (IOException e) { - return valueOf(1); - } + // Returns 0 for success, 1 in case of error when writing pack. + Callable packRefs = () -> { + syncPoint.await(); + try { + gc.packRefs(); + return 0; + } catch (IOException e) { + return 1; } }; ExecutorService pool = Executors.newFixedThreadPool(2); try { Future p1 = pool.submit(packRefs); Future p2 = pool.submit(packRefs); - assertEquals(1, p1.get().intValue() + p2.get().intValue()); + assertThat(p1.get() + p2.get(), lessThanOrEqualTo(1)); } finally { pool.shutdown(); pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS); 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 5d66a4fbd..e03bd8723 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 @@ -914,8 +914,24 @@ protected void writeFile(String name, byte[] content) throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name)); byte[] digest = Constants.newMessageDigest().digest(content); - packedRefs.compareAndSet(oldPackedList, new PackedRefList(refs, - lck.getCommitSnapshot(), ObjectId.fromRaw(digest))); + PackedRefList newPackedList = new PackedRefList( + refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest)); + + // This thread holds the file lock, so no other thread or process should + // be able to modify the packed-refs file on disk. If the list changed, + // it means something is very wrong, so throw an exception. + // + // However, we can't use a naive compareAndSet to check whether the + // update was successful, because another thread might _read_ the + // packed refs file that was written out by this thread while holding + // the lock, and update the packedRefs reference to point to that. So + // compare the actual contents instead. + PackedRefList afterUpdate = packedRefs.updateAndGet( + p -> p.id.equals(oldPackedList.id) ? newPackedList : p); + if (!afterUpdate.id.equals(newPackedList.id)) { + throw new ObjectWritingException( + MessageFormat.format(JGitText.get().unableToWrite, name)); + } } }.writePackedRefs(); }