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