Fix atomic lock file creation on NFS
FS_POSIX.createNewFile(File) failed to properly implement atomic file
creation on NFS using the algorithm [1]:
- name of the hard link must be unique to prevent that two processes
using different NFS clients try to create the same link. This would
render nlink useless to detect if there was a race.
- the hard link must be retained for the lifetime of the file since we
don't know when the state of the involved NFS clients will be
synchronized. This depends on NFS configuration options.
To fix these issues we need to change the signature of createNewFile
which would break API. Hence deprecate the old method
FS.createNewFile(File) and add a new method createNewFileAtomic(File).
The new method returns a LockToken which needs to be retained by the
caller (LockFile) until all involved NFS clients synchronized their
state. Since we don't know when the NFS caches are synchronized we need
to retain the token until the corresponding file is no longer needed.
The LockToken must be closed after the LockFile using it has been
committed or unlocked. On Posix, if core.supportsAtomicCreateNewFile =
false this will delete the hard link which guarded the atomic creation
of the file. When acquiring the lock fails ensure that the hard link is
removed.
[1] https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html
also see file creation flag O_EXCL in
http://man7.org/linux/man-pages/man2/open.2.html
Change-Id: I84fcb16143a5f877e9b08c6ee0ff8fa4ea68a90d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
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 16f184b..873cf52 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 @@
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 @@
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:
@@ -667,6 +669,7 @@
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 4b7459c..9ea0aa5 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 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 String expectedPktLineWithService;
/***/ public String expectedReceivedContentType;
/***/ public String expectedReportForRefNotReceived;
+ /***/ public String failedAtomicFileCreation;
/***/ public String failedToDetermineFilterDefinition;
/***/ public String failedUpdatingRefs;
/***/ public String failureDueToOneOfTheFollowing;
@@ -726,6 +728,7 @@
/***/ 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/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java
index 584c471..15c5280 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
@@ -64,7 +64,10 @@
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.
@@ -77,6 +80,7 @@
* name.
*/
public class LockFile {
+ private final static Logger LOG = LoggerFactory.getLogger(LockFile.class);
/**
* Unlock the given file.
@@ -132,6 +136,8 @@
private FileSnapshot commitSnapshot;
+ private LockToken token;
+
/**
* Create a new lock for any file.
*
@@ -170,7 +176,8 @@
*/
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);
@@ -178,6 +185,8 @@
unlock();
throw ioe;
}
+ } else {
+ closeToken();
}
return haveLck;
}
@@ -458,6 +467,7 @@
try {
FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE);
haveLck = false;
+ closeToken();
return true;
} catch (IOException e) {
unlock();
@@ -465,6 +475,13 @@
}
}
+ private void closeToken() {
+ if (token != null) {
+ token.close();
+ token = null;
+ }
+ }
+
private void saveStatInformation() {
if (needSnapshot)
commitSnapshot = FileSnapshot.save(lck);
@@ -503,8 +520,9 @@
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;
}
@@ -514,7 +532,10 @@
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/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
index 98e7321..6b537a4 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 @@
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.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.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;
@@ -800,14 +804,79 @@
* the file to be created
* @return <code>true</code> if the file was created, <code>false</code> 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<Path> link;
+
+ LockToken(boolean isCreated, Optional<Path> 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$
+ : "<null>]"); //$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 <code>false</code>
+ *
+ * @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#relativize(String, String)}.
*
* @param base
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 9f273d1..607e078 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.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 @@
* 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;
@@ -395,4 +403,70 @@
}
}
}
+
+ /**
+ * {@inheritDoc}
+ * <p>
+ * 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());
+ }
}
