From bd5e4eabc2a949dbce4d968d338106d46ff223df Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Fri, 15 Mar 2013 11:08:17 +0100 Subject: [PATCH] Fix GC for FileRepo in case packfile renames fail Only on Windows the rename operation which renames temporary Packfiles (and index-files and bitmap-files) sometime fails. This happens only when renaming a temporary Packfile to a Packfile which already exists. Such situations occur if you run GC twice on a repo without modifying the repo inbetween. In such situations there was bug in GC which led to a corrupted repo whithout any packfiles anymore. This commit fixes the problem by introducing a utility method which renames a file and throws an IOException if it fails. This method also takes care to repeat a failing rename if our FS class has found out we are running on a platform with a unreliable File.renameTo() method. I am searching for a better solution because even with this utility method in hand a GC on a already GC'ed repo will fail on Windows. But at least with this fix we will not produce corrupted repos anymore. Bug: 389305 Change-Id: Iac1ab3e0b8c419c90404f2e2f3559672eb8f6d28 Signed-off-by: Christian Halstrick Signed-off-by: Matthias Sohn --- .../jgit/internal/storage/file/GCTest.java | 7 ++++ .../jgit/internal/storage/file/GC.java | 18 ++++++++-- .../src/org/eclipse/jgit/util/FileUtils.java | 33 +++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java index bc60f6488..d5c8f12ef 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GCTest.java @@ -435,6 +435,13 @@ public class GCTest extends LocalDiskRepositoryTestCase { assertEquals(0, stats.numberOfLooseObjects); assertEquals(4, stats.numberOfPackedObjects); assertEquals(1, stats.numberOfPackFiles); + + // Do the gc again and check that it hasn't changed anything + gc.gc(); + stats = gc.getStatistics(); + assertEquals(0, stats.numberOfLooseObjects); + assertEquals(4, stats.numberOfPackedObjects); + assertEquals(1, stats.numberOfPackFiles); } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index 22fa82704..bc14d88d6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -735,11 +735,21 @@ public class GC { // rename the temporary files to real files File realPack = nameFor(id, ".pack"); //$NON-NLS-1$ + + // if the packfile already exists (because we are rewriting a + // packfile for the same set of objects maybe with different + // PackConfig) then make sure we get rid of all handles on the file. + // Windows will not allow for rename otherwise. + if (realPack.exists()) + for (PackFile p : repo.getObjectDatabase().getPacks()) + if (realPack.getPath().equals(p.getPackFile().getPath())) { + p.close(); + break; + } tmpPack.setReadOnly(); boolean delete = true; try { - if (!tmpPack.renameTo(realPack)) - return null; + FileUtils.rename(tmpPack, realPack); delete = false; for (Map.Entry tmpEntry : tmpExts.entrySet()) { File tmpExt = tmpEntry.getValue(); @@ -747,7 +757,9 @@ public class GC { File realExt = nameFor( id, "." + tmpEntry.getKey().getExtension()); //$NON-NLS-1$ - if (!tmpExt.renameTo(realExt)) { + try { + FileUtils.rename(tmpExt, realExt); + } catch (IOException e) { File newExt = new File(realExt.getParentFile(), realExt.getName() + ".new"); //$NON-NLS-1$ if (!tmpExt.renameTo(newExt)) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java index 67f371b61..acc1a2c31 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java @@ -167,6 +167,39 @@ public class FileUtils { } } + /** + * Rename a file or folder. If the rename fails and if we are running on a + * filesystem where it makes sense to repeat a failing rename then repeat + * the rename operation up to 9 times with 100ms sleep time between two + * calls + * + * @see FS#retryFailedLockFileCommit() + * @param src + * the old {@code File} + * @param dst + * the new {@code File} + * @throws IOException + * if the rename has failed + */ + public static void rename(final File src, final File dst) + throws IOException { + int attempts = FS.DETECTED.retryFailedLockFileCommit() ? 10 : 1; + while (--attempts >= 0) { + if (src.renameTo(dst)) + return; + try { + Thread.sleep(100); + } catch (InterruptedException e) { + throw new IOException(MessageFormat.format( + JGitText.get().renameFileFailed, src.getAbsolutePath(), + dst.getAbsolutePath())); + } + } + throw new IOException(MessageFormat.format( + JGitText.get().renameFileFailed, src.getAbsolutePath(), + dst.getAbsolutePath())); + } + /** * Creates the directory named by this abstract pathname. *