diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java index daef91d53..0f26b0fa6 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java @@ -46,6 +46,7 @@ package org.eclipse.jgit.internal.storage.file; import static org.eclipse.jgit.junit.Assert.assertEquals; +import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -758,11 +759,11 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase { // Check that the involved refs are the same despite the failure assertExists(false, toName); if (!toLock.equals(toName)) - assertExists(false, toName + ".lock"); - assertExists(true, toLock + ".lock"); + assertExists(false, toName + LOCK_SUFFIX); + assertExists(true, toLock + LOCK_SUFFIX); if (!toLock.equals(fromName)) - assertExists(false, "logs/" + fromName + ".lock"); - assertExists(false, "logs/" + toName + ".lock"); + assertExists(false, "logs/" + fromName + LOCK_SUFFIX); + assertExists(false, "logs/" + toName + LOCK_SUFFIX); assertEquals(oldHeadId, db.resolve(Constants.HEAD)); assertEquals(oldfromId, db.resolve(fromName)); assertNull(db.resolve(toName)); diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 8a24587e2..2d34f5717 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -38,5 +38,21 @@ + + + + + + + + + + + + + + + + diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 4d2e38e7e..8fce30a2a 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -119,6 +119,7 @@ checkoutUnexpectedResult=Checkout returned unexpected result {0} classCastNotA=Not a {0} cloneNonEmptyDirectory=Destination path "{0}" already exists and is not an empty directory closed=closed +closeLockTokenFailed=Closing LockToken ''{0}'' failed collisionOn=Collision on {0} commandRejectedByHook=Rejected by "{0}" hook.\n{1} commandWasCalledInTheWrongState=Command {0} was called in the wrong state @@ -284,6 +285,7 @@ expectedLessThanGot=expected less than ''{0}'', got ''{1}'' expectedPktLineWithService=expected pkt-line with ''# service=-'', got ''{0}'' expectedReceivedContentType=expected Content-Type {0}; received Content-Type {1} expectedReportForRefNotReceived={0}: expected report for ref {1} not received +failedAtomicFileCreation=Atomic file creation failed, number of hard links to file {0} was not 2 but {1}" failedToDetermineFilterDefinition=An exception occured while determining filter definitions failedUpdatingRefs=failed updating refs failureDueToOneOfTheFollowing=Failure due to one of the following: @@ -669,6 +671,7 @@ unknownObjectType2=unknown unknownRepositoryFormat=Unknown repository format unknownRepositoryFormat2=Unknown repository format "{0}"; expected "0". unknownZlibError=Unknown zlib error. +unlockLockFileFailed=Unlocking LockFile ''{0}'' failed unmergedPath=Unmerged path: {0} unmergedPaths=Repository contains unmerged paths unpackException=Exception while parsing pack stream diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 4f74adfad..a4a970e24 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -177,6 +177,7 @@ public class JGitText extends TranslationBundle { /***/ public String checkoutUnexpectedResult; /***/ public String classCastNotA; /***/ public String cloneNonEmptyDirectory; + /***/ public String closeLockTokenFailed; /***/ public String closed; /***/ public String collisionOn; /***/ public String commandRejectedByHook; @@ -343,6 +344,7 @@ public class JGitText extends TranslationBundle { /***/ public String expectedPktLineWithService; /***/ public String expectedReceivedContentType; /***/ public String expectedReportForRefNotReceived; + /***/ public String failedAtomicFileCreation; /***/ public String failedToDetermineFilterDefinition; /***/ public String failedUpdatingRefs; /***/ public String failureDueToOneOfTheFollowing; @@ -728,6 +730,7 @@ public class JGitText extends TranslationBundle { /***/ public String unknownRepositoryFormat; /***/ public String unknownRepositoryFormat2; /***/ public String unknownZlibError; + /***/ public String unlockLockFileFailed; /***/ public String unmergedPath; /***/ public String unmergedPaths; /***/ public String unpackException; 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 b3d923c41..7ff209fb8 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 @@ -54,6 +54,7 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.nio.channels.Channels; import java.nio.channels.FileChannel; +import java.nio.file.DirectoryNotEmptyException; import java.nio.file.DirectoryStream; import java.nio.file.Files; import java.nio.file.Path; @@ -940,6 +941,8 @@ public class GC { private void delete(Path d) { try { Files.delete(d); + } catch (DirectoryNotEmptyException e) { + // Don't log } catch (IOException e) { LOG.error(MessageFormat.format(JGitText.get().cannotDeleteFile, d), e); 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 5af93cf2c..15c528051 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 @@ -44,6 +44,8 @@ package org.eclipse.jgit.internal.storage.file; +import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX; + import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -62,7 +64,10 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.FS.LockToken; import org.eclipse.jgit.util.FileUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Git style file locking and replacement. @@ -75,7 +80,7 @@ import org.eclipse.jgit.util.FileUtils; * name. */ public class LockFile { - static final String SUFFIX = ".lock"; //$NON-NLS-1$ + private final static Logger LOG = LoggerFactory.getLogger(LockFile.class); /** * Unlock the given file. @@ -105,14 +110,15 @@ public class LockFile { * @return lock file */ static File getLockFile(File file) { - return new File(file.getParentFile(), file.getName() + SUFFIX); + return new File(file.getParentFile(), + file.getName() + LOCK_SUFFIX); } /** Filter to skip over active lock files when listing a directory. */ static final FilenameFilter FILTER = new FilenameFilter() { @Override public boolean accept(File dir, String name) { - return !name.endsWith(SUFFIX); + return !name.endsWith(LOCK_SUFFIX); } }; @@ -130,6 +136,8 @@ public class LockFile { private FileSnapshot commitSnapshot; + private LockToken token; + /** * Create a new lock for any file. * @@ -168,7 +176,8 @@ public class LockFile { */ public boolean lock() throws IOException { FileUtils.mkdirs(lck.getParentFile(), true); - if (FS.DETECTED.createNewFile(lck)) { + token = FS.DETECTED.createNewFileAtomic(lck); + if (token.isCreated()) { haveLck = true; try { os = new FileOutputStream(lck); @@ -176,6 +185,8 @@ public class LockFile { unlock(); throw ioe; } + } else { + closeToken(); } return haveLck; } @@ -456,6 +467,7 @@ public class LockFile { try { FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE); haveLck = false; + closeToken(); return true; } catch (IOException e) { unlock(); @@ -463,6 +475,13 @@ public class LockFile { } } + private void closeToken() { + if (token != null) { + token.close(); + token = null; + } + } + private void saveStatInformation() { if (needSnapshot) commitSnapshot = FileSnapshot.save(lck); @@ -501,8 +520,9 @@ public class LockFile { if (os != null) { try { os.close(); - } catch (IOException ioe) { - // Ignore this + } catch (IOException e) { + LOG.error(MessageFormat + .format(JGitText.get().unlockLockFileFailed, lck), e); } os = null; } @@ -512,7 +532,10 @@ public class LockFile { try { FileUtils.delete(lck, FileUtils.RETRY); } catch (IOException e) { - // couldn't delete the file even after retry. + LOG.error(MessageFormat + .format(JGitText.get().unlockLockFileFailed, lck), e); + } finally { + closeToken(); } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ReflogWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ReflogWriter.java index 24d2c790e..892c1c85a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ReflogWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ReflogWriter.java @@ -47,6 +47,7 @@ package org.eclipse.jgit.internal.storage.file; import static org.eclipse.jgit.lib.Constants.HEAD; import static org.eclipse.jgit.lib.Constants.LOGS; +import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX; import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.Constants.R_REFS; import static org.eclipse.jgit.lib.Constants.R_REMOTES; @@ -86,7 +87,7 @@ public class ReflogWriter { * @return the name of the ref's lock ref */ public static String refLockFor(final String name) { - return name + LockFile.SUFFIX; + return name + LOCK_SUFFIX; } private final Repository parent; @@ -287,4 +288,4 @@ public class ReflogWriter { || refName.startsWith(R_REMOTES) // || refName.equals(R_STASH); } -} \ No newline at end of file +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java index ff80672f8..943cb9c5d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java @@ -665,6 +665,13 @@ public final class Constants { public static final ObjectId EMPTY_BLOB_ID = ObjectId .fromString("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"); + /** + * Suffix of lock file name + * + * @since 4.8 + */ + public static final String LOCK_SUFFIX = ".lock"; //$NON-NLS-1$ + private Constants() { // Hide the default constructor } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index 1f2ab9df8..bd23ab988 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -48,6 +48,8 @@ package org.eclipse.jgit.lib; +import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX; + import java.io.BufferedOutputStream; import java.io.File; import java.io.FileNotFoundException; @@ -1320,10 +1322,12 @@ public abstract class Repository implements AutoCloseable { */ public static boolean isValidRefName(final String refName) { final int len = refName.length(); - if (len == 0) + if (len == 0) { return false; - if (refName.endsWith(".lock")) //$NON-NLS-1$ + } + if (refName.endsWith(LOCK_SUFFIX)) { return false; + } // Refs may be stored as loose files so invalid paths // on the local system must also be invalid refs. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportSftp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportSftp.java index c46f94b7d..99e4809cf 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportSftp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportSftp.java @@ -43,6 +43,8 @@ package org.eclipse.jgit.transport; +import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX; + import java.io.BufferedReader; import java.io.FileNotFoundException; import java.io.IOException; @@ -341,7 +343,7 @@ public class TransportSftp extends SshTransport implements WalkTransport { @Override void writeFile(final String path, final byte[] data) throws IOException { - final String lock = path + ".lock"; //$NON-NLS-1$ + final String lock = path + LOCK_SUFFIX; try { super.writeFile(lock, data); try { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index 3e0b41af3..ed5b87d5c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java @@ -45,6 +45,7 @@ package org.eclipse.jgit.util; import java.io.BufferedReader; import java.io.ByteArrayInputStream; +import java.io.Closeable; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -52,6 +53,8 @@ import java.io.InputStreamReader; import java.io.OutputStream; import java.io.PrintStream; import java.nio.charset.Charset; +import java.nio.file.Files; +import java.nio.file.Path; import java.security.AccessController; import java.security.PrivilegedAction; import java.text.MessageFormat; @@ -59,6 +62,7 @@ import java.util.Arrays; import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; @@ -806,13 +810,78 @@ public abstract class FS { * the file to be created * @return true if the file was created, false if * the file already existed - * @throws IOException + * @throws java.io.IOException + * @deprecated use {@link #createNewFileAtomic(File)} instead * @since 4.5 */ + @Deprecated public boolean createNewFile(File path) throws IOException { return path.createNewFile(); } + /** + * A token representing a file created by + * {@link #createNewFileAtomic(File)}. The token must be retained until the + * file has been deleted in order to guarantee that the unique file was + * created atomically. As soon as the file is no longer needed the lock + * token must be closed. + * + * @since 4.7 + */ + public static class LockToken implements Closeable { + private boolean isCreated; + + private Optional link; + + LockToken(boolean isCreated, Optional link) { + this.isCreated = isCreated; + this.link = link; + } + + /** + * @return {@code true} if the file was created successfully + */ + public boolean isCreated() { + return isCreated; + } + + @Override + public void close() { + if (link.isPresent()) { + try { + Files.delete(link.get()); + } catch (IOException e) { + LOG.error(MessageFormat.format(JGitText.get().closeLockTokenFailed, + this), e); + } + } + } + + @Override + public String toString() { + return "LockToken [lockCreated=" + isCreated + //$NON-NLS-1$ + ", link=" //$NON-NLS-1$ + + (link.isPresent() ? link.get().getFileName() + "]" //$NON-NLS-1$ + : "]"); //$NON-NLS-1$ + } + } + + /** + * Create a new file. See {@link java.io.File#createNewFile()}. Subclasses + * of this class may take care to provide a safe implementation for this + * even if {@link #supportsAtomicCreateNewFile()} is false + * + * @param path + * the file to be created + * @return LockToken this token must be closed after the created file was + * deleted + * @throws IOException + * @since 4.7 + */ + public LockToken createNewFileAtomic(File path) throws IOException { + return new LockToken(path.createNewFile(), Optional.empty()); + } + /** * See {@link FileUtils#relativizePath(String, String, String, boolean)}. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java index 76aa69776..607e07860 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java @@ -52,14 +52,19 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.PosixFilePermission; +import java.text.MessageFormat; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.UUID; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.api.errors.JGitInternalException; import org.eclipse.jgit.errors.CommandFailedException; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Repository; @@ -360,9 +365,12 @@ public class FS_POSIX extends FS { * multiple clients manage to create the same lock file nlink would be * greater than 2 showing the error. * - * @see https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html + * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html" + * + * @deprecated use {@link FS_POSIX#createNewFileAtomic(File)} instead * @since 4.5 */ + @Deprecated public boolean createNewFile(File lock) throws IOException { if (!lock.createNewFile()) { return false; @@ -371,22 +379,94 @@ public class FS_POSIX extends FS { return true; } Path lockPath = lock.toPath(); - Path link = Files.createLink(Paths.get(lock.getAbsolutePath() + ".lnk"), //$NON-NLS-1$ - lockPath); + Path link = null; try { + link = Files.createLink( + Paths.get(lock.getAbsolutePath() + ".lnk"), //$NON-NLS-1$ + lockPath); Integer nlink = (Integer) (Files.getAttribute(lockPath, "unix:nlink")); //$NON-NLS-1$ - if (nlink != 2) { + if (nlink > 2) { LOG.warn("nlink of link to lock file {0} was not 2 but {1}", //$NON-NLS-1$ lock.getPath(), nlink); return false; + } else if (nlink < 2) { + supportsUnixNLink = false; } return true; } catch (UnsupportedOperationException | IllegalArgumentException e) { supportsUnixNLink = false; return true; } finally { - Files.delete(link); + if (link != null) { + Files.delete(link); + } } } + + /** + * {@inheritDoc} + *

+ * An implementation of the File#createNewFile() semantics which can create + * a unique file atomically also on NFS. If the config option + * {@code core.supportsAtomicCreateNewFile = true} (which is the default) + * then simply File#createNewFile() is called. + * + * But if {@code core.supportsAtomicCreateNewFile = false} then after + * successful creation of the lock file a hard link to that lock file is + * created and the attribute nlink of the lock file is checked to be 2. If + * multiple clients manage to create the same lock file nlink would be + * greater than 2 showing the error. The hard link needs to be retained + * until the corresponding file is no longer needed in order to prevent that + * another process can create the same file concurrently using another NFS + * client which might not yet see the file due to caching. + * + * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html" + * @param file + * the unique file to be created atomically + * @return LockToken this lock token must be held until the file is no + * longer needed + * @throws IOException + * @since 5.0 + */ + @Override + public LockToken createNewFileAtomic(File file) throws IOException { + if (!file.createNewFile()) { + return token(false, null); + } + if (supportsAtomicCreateNewFile() || !supportsUnixNLink) { + return token(true, null); + } + Path link = null; + Path path = file.toPath(); + try { + link = Files.createLink(Paths.get(uniqueLinkPath(file)), path); + Integer nlink = (Integer) (Files.getAttribute(path, + "unix:nlink")); //$NON-NLS-1$ + if (nlink.intValue() > 2) { + LOG.warn(MessageFormat.format( + JGitText.get().failedAtomicFileCreation, path, nlink)); + return token(false, link); + } else if (nlink.intValue() < 2) { + supportsUnixNLink = false; + } + return token(true, link); + } catch (UnsupportedOperationException | IllegalArgumentException e) { + supportsUnixNLink = false; + return token(true, link); + } + } + + private static LockToken token(boolean created, @Nullable Path p) { + return ((p != null) && Files.exists(p)) + ? new LockToken(created, Optional.of(p)) + : new LockToken(created, Optional.empty()); + } + + private static String uniqueLinkPath(File file) { + UUID id = UUID.randomUUID(); + return file.getAbsolutePath() + "." //$NON-NLS-1$ + + Long.toHexString(id.getMostSignificantBits()) + + Long.toHexString(id.getLeastSignificantBits()); + } }