From 3fc93f8a562e96d354546ad6ff8c9dd56709c5e5 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Thu, 23 Apr 2015 02:11:00 +0200 Subject: [PATCH] Rename files using NIO2 atomic rename Bug: 319233 Change-Id: I5137212f5cd3195a52f90ed5e4ce3cf194a13efd Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/api/ApplyCommand.java | 9 ++- .../eclipse/jgit/api/StashDropCommand.java | 11 +-- .../jgit/dircache/DirCacheCheckout.java | 10 +-- .../jgit/internal/storage/file/GC.java | 53 +++++++------- .../jgit/internal/storage/file/LockFile.java | 70 ++++++------------- .../storage/file/ObjectDirectory.java | 19 ++++- .../file/ObjectDirectoryPackParser.java | 14 ++-- .../storage/file/RefDirectoryRename.java | 23 +++++- 8 files changed, 112 insertions(+), 97 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/ApplyCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/ApplyCommand.java index 6a945e4d3..676ae0300 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/ApplyCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/ApplyCommand.java @@ -47,6 +47,7 @@ import java.io.FileOutputStream; import java.io.FileWriter; import java.io.IOException; import java.io.InputStream; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.util.ArrayList; import java.util.List; @@ -141,9 +142,13 @@ public class ApplyCommand extends GitCommand { case RENAME: f = getFile(fh.getOldPath(), false); File dest = getFile(fh.getNewPath(), false); - if (!f.renameTo(dest)) + try { + FileUtils.rename(f, dest, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { throw new PatchApplyException(MessageFormat.format( - JGitText.get().renameFileFailed, f, dest)); + JGitText.get().renameFileFailed, f, dest), e); + } break; case COPY: f = getFile(fh.getOldPath(), false); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/StashDropCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/StashDropCommand.java index 7923fd49b..f6903be05 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/StashDropCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/StashDropCommand.java @@ -46,6 +46,7 @@ import static org.eclipse.jgit.lib.Constants.R_STASH; import java.io.File; import java.io.IOException; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.util.List; @@ -220,12 +221,14 @@ public class StashDropCommand extends GitCommand { entry.getWho(), entry.getComment()); entryId = entry.getNewId(); } - if (!stashLockFile.renameTo(stashFile)) { - FileUtils.delete(stashFile); - if (!stashLockFile.renameTo(stashFile)) + try { + FileUtils.rename(stashLockFile, stashFile, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { throw new JGitInternalException(MessageFormat.format( JGitText.get().renameFileFailed, - stashLockFile.getPath(), stashFile.getPath())); + stashLockFile.getPath(), stashFile.getPath()), + e); } } catch (IOException e) { throw new JGitInternalException(JGitText.get().stashDropFailed, e); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java index 4eb688170..a1e1d15ac 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java @@ -46,6 +46,7 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStream; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.util.ArrayList; import java.util.HashMap; @@ -1319,11 +1320,12 @@ public class DirCacheCheckout { if (deleteRecursive && f.isDirectory()) { FileUtils.delete(f, FileUtils.RECURSIVE); } - FileUtils.rename(tmpFile, f); + FileUtils.rename(tmpFile, f, StandardCopyOption.ATOMIC_MOVE); } catch (IOException e) { - throw new IOException(MessageFormat.format( - JGitText.get().renameFileFailed, tmpFile.getPath(), - f.getPath())); + throw new IOException( + MessageFormat.format(JGitText.get().renameFileFailed, + tmpFile.getPath(), f.getPath()), + e); } finally { if (tmpFile.exists()) { FileUtils.delete(tmpFile); 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 7b3966de1..a5c95b3bc 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 @@ -53,6 +53,7 @@ import java.io.IOException; import java.io.OutputStream; import java.nio.channels.Channels; import java.nio.channels.FileChannel; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.text.ParseException; import java.util.ArrayList; @@ -789,39 +790,33 @@ public class GC { break; } tmpPack.setReadOnly(); - boolean delete = true; - try { - FileUtils.rename(tmpPack, realPack); - delete = false; - for (Map.Entry tmpEntry : tmpExts.entrySet()) { - File tmpExt = tmpEntry.getValue(); - tmpExt.setReadOnly(); - - File realExt = nameFor( - id, "." + tmpEntry.getKey().getExtension()); //$NON-NLS-1$ - try { - FileUtils.rename(tmpExt, realExt); - } catch (IOException e) { - File newExt = new File(realExt.getParentFile(), - realExt.getName() + ".new"); //$NON-NLS-1$ - if (!tmpExt.renameTo(newExt)) - newExt = tmpExt; - throw new IOException(MessageFormat.format( - JGitText.get().panicCantRenameIndexFile, newExt, - realExt)); - } - } - } finally { - if (delete) { - if (tmpPack.exists()) - tmpPack.delete(); - for (File tmpExt : tmpExts.values()) { - if (tmpExt.exists()) - tmpExt.delete(); + FileUtils.rename(tmpPack, realPack, StandardCopyOption.ATOMIC_MOVE); + for (Map.Entry tmpEntry : tmpExts.entrySet()) { + File tmpExt = tmpEntry.getValue(); + tmpExt.setReadOnly(); + + File realExt = nameFor(id, + "." + tmpEntry.getKey().getExtension()); //$NON-NLS-1$ + try { + FileUtils.rename(tmpExt, realExt, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { + File newExt = new File(realExt.getParentFile(), + realExt.getName() + ".new"); //$NON-NLS-1$ + try { + FileUtils.rename(tmpExt, newExt, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e2) { + newExt = tmpExt; + e = e2; } + throw new IOException(MessageFormat.format( + JGitText.get().panicCantRenameIndexFile, newExt, + realExt), e); } } + return repo.getObjectDatabase().openPack(realPack); } finally { if (tmpPack != null && tmpPack.exists()) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index e23ca741b..ce9677a62 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java @@ -54,6 +54,7 @@ import java.io.OutputStream; import java.nio.ByteBuffer; import java.nio.channels.Channels; import java.nio.channels.FileChannel; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import org.eclipse.jgit.errors.LockFailedException; @@ -128,8 +129,6 @@ public class LockFile { private FileSnapshot commitSnapshot; - private final FS fs; - /** * Create a new lock for any file. * @@ -138,11 +137,24 @@ public class LockFile { * @param fs * the file system abstraction which will be necessary to perform * certain file system operations. + * @deprecated use {@link LockFile#LockFile(File)} instead */ + @Deprecated public LockFile(final File f, final FS fs) { ref = f; lck = getLockFile(ref); - this.fs = fs; + } + + /** + * Create a new lock for any file. + * + * @param f + * the file that will be locked. + * @since 4.2 + */ + public LockFile(final File f) { + ref = f; + lck = getLockFile(ref); } /** @@ -441,56 +453,14 @@ public class LockFile { } saveStatInformation(); - if (lck.renameTo(ref)) { + try { + FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE); haveLck = false; return true; + } catch (IOException e) { + unlock(); + return false; } - if (!ref.exists() || deleteRef()) { - if (renameLock()) { - haveLck = false; - return true; - } - } - unlock(); - return false; - } - - private boolean deleteRef() { - if (!fs.retryFailedLockFileCommit()) - return ref.delete(); - - // File deletion fails on windows if another thread is - // concurrently reading the same file. So try a few times. - // - for (int attempts = 0; attempts < 10; attempts++) { - if (ref.delete()) - return true; - try { - Thread.sleep(100); - } catch (InterruptedException e) { - return false; - } - } - return false; - } - - private boolean renameLock() { - if (!fs.retryFailedLockFileCommit()) - return lck.renameTo(ref); - - // File renaming fails on windows if another thread is - // concurrently reading the same file. So try a few times. - // - for (int attempts = 0; attempts < 10; attempts++) { - if (lck.renameTo(ref)) - return true; - try { - Thread.sleep(100); - } catch (InterruptedException e) { - return false; - } - } - return false; } private void saveStatInformation() { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index bd1d488d9..ea8052851 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -52,6 +52,9 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileReader; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Arrays; @@ -608,10 +611,16 @@ public class ObjectDirectory extends FileObjectDatabase { FileUtils.delete(tmp, FileUtils.RETRY); return InsertLooseObjectResult.EXISTS_LOOSE; } - if (tmp.renameTo(dst)) { + try { + Files.move(tmp.toPath(), dst.toPath(), + StandardCopyOption.ATOMIC_MOVE); dst.setReadOnly(); unpackedObjectCache.add(id); return InsertLooseObjectResult.INSERTED; + } catch (AtomicMoveNotSupportedException e) { + LOG.error(e.getMessage(), e); + } catch (IOException e) { + // ignore } // Maybe the directory doesn't exist yet as the object @@ -619,10 +628,16 @@ public class ObjectDirectory extends FileObjectDatabase { // try the rename first as the directory likely does exist. // FileUtils.mkdir(dst.getParentFile(), true); - if (tmp.renameTo(dst)) { + try { + Files.move(tmp.toPath(), dst.toPath(), + StandardCopyOption.ATOMIC_MOVE); dst.setReadOnly(); unpackedObjectCache.add(id); return InsertLooseObjectResult.INSERTED; + } catch (AtomicMoveNotSupportedException e) { + LOG.error(e.getMessage(), e); + } catch (IOException e) { + LOG.debug(e.getMessage(), e); } if (!createDuplicate && has(id)) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java index 1c076ee09..2e6c245ea 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java @@ -50,6 +50,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.RandomAccessFile; +import java.nio.file.StandardCopyOption; import java.security.MessageDigest; import java.text.MessageFormat; import java.util.Arrays; @@ -476,20 +477,25 @@ public class ObjectDirectoryPackParser extends PackParser { } } - if (!tmpPack.renameTo(finalPack)) { + try { + FileUtils.rename(tmpPack, finalPack, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { cleanupTemporaryFiles(); keep.unlock(); throw new IOException(MessageFormat.format( - JGitText.get().cannotMovePackTo, finalPack)); + JGitText.get().cannotMovePackTo, finalPack), e); } - if (!tmpIdx.renameTo(finalIdx)) { + try { + FileUtils.rename(tmpIdx, finalIdx, StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { cleanupTemporaryFiles(); keep.unlock(); if (!finalPack.delete()) finalPack.deleteOnExit(); throw new IOException(MessageFormat.format( - JGitText.get().cannotMoveIndexTo, finalIdx)); + JGitText.get().cannotMoveIndexTo, finalIdx), e); } try { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryRename.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryRename.java index ba4a63d7f..4b803a514 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryRename.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryRename.java @@ -46,6 +46,8 @@ package org.eclipse.jgit.internal.storage.file; import java.io.File; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; +import java.nio.file.StandardCopyOption; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -54,6 +56,8 @@ import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.FileUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Rename any reference stored by {@link RefDirectory}. @@ -66,6 +70,9 @@ import org.eclipse.jgit.util.FileUtils; * directory that happens to match the source name. */ class RefDirectoryRename extends RefRename { + private static final Logger LOG = LoggerFactory + .getLogger(RefDirectoryRename.class); + private final RefDirectory refdb; /** @@ -201,13 +208,25 @@ class RefDirectoryRename extends RefRename { } private static boolean rename(File src, File dst) { - if (src.renameTo(dst)) + try { + FileUtils.rename(src, dst, StandardCopyOption.ATOMIC_MOVE); return true; + } catch (AtomicMoveNotSupportedException e) { + LOG.error(e.getMessage(), e); + } catch (IOException e) { + // ignore + } File dir = dst.getParentFile(); if ((dir.exists() || !dir.mkdirs()) && !dir.isDirectory()) return false; - return src.renameTo(dst); + try { + FileUtils.rename(src, dst, StandardCopyOption.ATOMIC_MOVE); + return true; + } catch (IOException e) { + LOG.error(e.getMessage(), e); + return false; + } } private boolean linkHEAD(RefUpdate target) {