From 1ed1e40387c859786cf1932d22fe0001fbd5abb6 Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Mon, 7 Aug 2017 14:26:46 +0200 Subject: [PATCH] Fix exception handling for opening bitmap index files When creating a new PackFile instance it is specified whether this pack has an associated bitmap index file or not. This information is cached and the public method getBitmapIndex() will always assume a bitmap index file must exist if the cached data tells so. But it may happen that the packfiles are repacked during a gc in a different process causing the packfile, bitmap-index and index file to be deleted. Since JGit still has an open FileHandle on the packfile this file is not really deleted and can still be accessed. But index and bitmap index file are deleted. Fix getBitmapIndex() to invalidate the cached packfile instance if such a situation occurs. This problem showed up when a gerrit server was serving repositories which where garbage collected with native git regularly. Fetch and clone commands for certain repositories failed permanently after a native git gc had deleted old bitmap index files. Change-Id: I8e620bec74dd3f310ba42024f9a657062f868f0e Signed-off-by: Matthias Sohn --- .../storage/file/GcConcurrentTest.java | 103 ++++++++++++++++++ .../jgit/internal/storage/file/PackFile.java | 13 ++- 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java index 07a7be7467..776226b4ef 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java @@ -45,8 +45,12 @@ package org.eclipse.jgit.internal.storage.file; import static java.lang.Integer.valueOf; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import java.io.IOException; +import java.util.Collection; +import java.util.Collections; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.Callable; import java.util.concurrent.CyclicBarrier; @@ -56,8 +60,14 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.storage.pack.PackWriter; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.EmptyProgressMonitor; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Sets; import org.eclipse.jgit.revwalk.RevBlob; +import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; public class GcConcurrentTest extends GcTestCase { @@ -116,4 +126,97 @@ public class GcConcurrentTest extends GcTestCase { pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS); } } + + @Test + public void repackAndGetStats() throws Exception { + TestRepository.BranchBuilder test = tr.branch("test"); + test.commit().add("a", "a").create(); + GC gc1 = new GC(tr.getRepository()); + gc1.setPackExpireAgeMillis(0); + gc1.gc(); + test.commit().add("b", "b").create(); + + // Create a new Repository instance and trigger a gc + // from that instance. Reusing the existing repo instance + // tr.getRepository() would not show the problem. + FileRepository r2 = new FileRepository( + tr.getRepository().getDirectory()); + GC gc2 = new GC(r2); + gc2.setPackExpireAgeMillis(0); + gc2.gc(); + + new GC(tr.getRepository()).getStatistics(); + } + + @Test + public void repackAndUploadPack() throws Exception { + TestRepository.BranchBuilder test = tr.branch("test"); + // RevCommit a = test.commit().add("a", "a").create(); + test.commit().add("a", "a").create(); + + GC gc1 = new GC(tr.getRepository()); + gc1.setPackExpireAgeMillis(0); + gc1.gc(); + + RevCommit b = test.commit().add("b", "b").create(); + + FileRepository r2 = new FileRepository( + tr.getRepository().getDirectory()); + GC gc2 = new GC(r2); + gc2.setPackExpireAgeMillis(0); + gc2.gc(); + + // Simulate parts of an UploadPack. This is the situation on + // server side (e.g. gerrit) when when clients are + // cloning/fetching while the server side repo's + // are gc'ed by an external process (e.g. scheduled + // native git gc) + try (PackWriter pw = new PackWriter(tr.getRepository())) { + pw.setUseBitmaps(true); + pw.preparePack(NullProgressMonitor.INSTANCE, Sets.of(b), + Collections. emptySet()); + new GC(tr.getRepository()).getStatistics(); + } + } + + PackFile getSinglePack(FileRepository r) { + Collection packs = r.getObjectDatabase().getPacks(); + assertEquals(1, packs.size()); + return packs.iterator().next(); + } + + @Test + public void repackAndCheckBitmapUsage() throws Exception { + // create a test repository with one commit and pack all objects. After + // packing create loose objects to trigger creation of a new packfile on + // the next gc + TestRepository.BranchBuilder test = tr.branch("test"); + test.commit().add("a", "a").create(); + FileRepository repository = tr.getRepository(); + GC gc1 = new GC(repository); + gc1.setPackExpireAgeMillis(0); + gc1.gc(); + String oldPackName = getSinglePack(repository).getPackName(); + RevCommit b = test.commit().add("b", "b").create(); + + // start the garbage collection on a new repository instance, + FileRepository repository2 = new FileRepository(repository.getDirectory()); + GC gc2 = new GC(repository2); + gc2.setPackExpireAgeMillis(0); + gc2.gc(); + String newPackName = getSinglePack(repository2).getPackName(); + // make sure gc() has caused creation of a new packfile + assertNotEquals(oldPackName, newPackName); + + // Even when asking again for the set of packfiles outdated data + // will be returned. As long as the repository can work on cached data + // it will do so and not detect that a new packfile exists. + assertNotEquals(getSinglePack(repository).getPackName(), newPackName); + + // Only when accessing object content it is required to rescan the pack + // directory and the new packfile will be detected. + repository.getObjectDatabase().open(b).getSize(); + assertEquals(getSinglePack(repository).getPackName(), newPackName); + assertNotNull(getSinglePack(repository).getBitmapIndex()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java index 038172f92d..b5889f2dd4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java @@ -1105,8 +1105,17 @@ public class PackFile implements Iterable { if (invalid || invalidBitmap) return null; if (bitmapIdx == null && hasExt(BITMAP_INDEX)) { - final PackBitmapIndex idx = PackBitmapIndex.open( - extFile(BITMAP_INDEX), idx(), getReverseIdx()); + final PackBitmapIndex idx; + try { + idx = PackBitmapIndex.open(extFile(BITMAP_INDEX), idx(), + getReverseIdx()); + } catch (FileNotFoundException e) { + // Once upon a time this bitmap file existed. Now it + // has been removed. Most likely an external gc has + // removed this packfile and the bitmap + invalidBitmap = true; + return null; + } // At this point, idx() will have set packChecksum. if (Arrays.equals(packChecksum, idx.packChecksum)) -- 2.39.5