Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorChristian Halstrick2017-11-14 16:08:56 +0000
committerChristian Halstrick2017-11-22 17:15:11 +0000
commit10e65cb4faf943d4a2a782ceef6f0f09934b150b (patch)
treed3bb1a9c34bd017ae043b923b4a72ce476dc580d
parent218cf3403de512f564aa74f18de56c97dd7852b4 (diff)
downloadjgit-10e65cb4faf943d4a2a782ceef6f0f09934b150b.tar.gz
jgit-10e65cb4faf943d4a2a782ceef6f0f09934b150b.tar.xz
jgit-10e65cb4faf943d4a2a782ceef6f0f09934b150b.zip
Fix LockFile semantics when running on NFS
When running on NFS there was a chance that JGits LockFile semantic is broken because File#createNewFile() may allow multiple clients to create the same file in parallel. This change provides a fix which is only used when the new config option core.supportsAtomicCreateNewFile is set to false. The default for this option is true. This option can only be set in the global or the system config file. The repository config file is not taken into account in this case. If the config option core.supportsAtomicCreateNewFile is true then File#createNewFile() is trusted and the behaviour doesn't change. But if core.supportsAtomicCreateNewFile is set to false then after successful creation of the lock file a hardlink 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. This expensive workaround is described in https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html section III.d) "Exclusive File Creation" Change-Id: I3d2cc48d8eb280d5f7039eb94da37804f903be6a
-rw-r--r--org.eclipse.jgit/.settings/.api_filters4
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java2
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java7
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java31
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java86
5 files changed, 127 insertions, 3 deletions
diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters
index 2c27b051b0..3830563495 100644
--- a/org.eclipse.jgit/.settings/.api_filters
+++ b/org.eclipse.jgit/.settings/.api_filters
@@ -1,9 +1,9 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<component id="org.eclipse.jgit" version="2">
<resource path="META-INF/MANIFEST.MF">
- <filter comment="non-breaking addition of exception classes needed to cleanly fix error handling in PackFile" id="924844039">
+ <filter id="924844039">
<message_arguments>
- <message_argument value="4.5.2"/>
+ <message_argument value="4.5.4"/>
<message_argument value="4.5.0"/>
</message_arguments>
</filter>
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 ce9677a62d..51af67e0bd 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
@@ -168,7 +168,7 @@ public class LockFile {
*/
public boolean lock() throws IOException {
FileUtils.mkdirs(lck.getParentFile(), true);
- if (lck.createNewFile()) {
+ if (FS.DETECTED.createNewFile(lck)) {
haveLck = true;
try {
os = new FileOutputStream(lck);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
index 9e3e0b78fd..e3f8ba5b5b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
@@ -290,6 +290,13 @@ public class ConfigConstants {
public static final String CONFIG_KEY_TRUSTFOLDERSTAT = "trustfolderstat";
/**
+ * The "supportsAtomicFileCreation" key in the "core section"
+ *
+ * @since 4.5
+ */
+ public static final String CONFIG_KEY_SUPPORTSATOMICFILECREATION = "supportsatomicfilecreation";
+
+ /**
* The "noprefix" key in the "diff section"
* @since 3.0
*/
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 a55b5c0ac5..e1fd1cb889 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -237,6 +237,21 @@ public abstract class FS {
public abstract boolean supportsExecute();
/**
+ * Does this file system support atomic file creation via
+ * java.io.File#createNewFile()? In certain environments (e.g. on NFS) it is
+ * not guaranteed that when two file system clients run createNewFile() in
+ * parallel only one will succeed. In such cases both clients may think they
+ * created a new file.
+ *
+ * @return true if this implementation support atomic creation of new
+ * Files by {@link File#createNewFile()}
+ * @since 4.5
+ */
+ public boolean supportsAtomicCreateNewFile() {
+ return true;
+ }
+
+ /**
* Does this operating system and JRE supports symbolic links. The
* capability to handle symbolic links is detected at runtime.
*
@@ -777,6 +792,22 @@ public abstract class FS {
}
/**
+ * Create a new file. See {@link 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 <code>true</code> if the file was created, <code>false</code> if
+ * the file already existed
+ * @throws IOException
+ * @since 4.5
+ */
+ public boolean createNewFile(File path) throws IOException {
+ return path.createNewFile();
+ }
+
+ /**
* 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 cb4868cc7a..4ecaf9c8ee 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
@@ -50,6 +50,7 @@ import java.io.PrintStream;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.Paths;
import java.nio.file.attribute.PosixFilePermission;
import java.util.ArrayList;
import java.util.Arrays;
@@ -58,8 +59,11 @@ import java.util.Set;
import org.eclipse.jgit.api.errors.JGitInternalException;
import org.eclipse.jgit.errors.CommandFailedException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ConfigConstants;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -74,6 +78,10 @@ public class FS_POSIX extends FS {
private static final int DEFAULT_UMASK = 0022;
private volatile int umask = -1;
+ private volatile boolean supportsUnixNLink = true;
+
+ private volatile Boolean supportsAtomicCreateNewFile;
+
/** Default constructor. */
protected FS_POSIX() {
}
@@ -91,6 +99,33 @@ public class FS_POSIX extends FS {
}
}
+ private void determineAtomicFileCreationSupport() {
+ // @TODO: enhance SystemReader to support this without copying code
+ Boolean ret = getAtomicFileCreationSupportOption(
+ SystemReader.getInstance().openUserConfig(null, this));
+ if (ret == null && StringUtils.isEmptyOrNull(SystemReader.getInstance()
+ .getenv(Constants.GIT_CONFIG_NOSYSTEM_KEY))) {
+ ret = getAtomicFileCreationSupportOption(
+ SystemReader.getInstance().openSystemConfig(null, this));
+ }
+ supportsAtomicCreateNewFile = (ret == null) || ret;
+ }
+
+ private Boolean getAtomicFileCreationSupportOption(FileBasedConfig config) {
+ try {
+ config.load();
+ String value = config.getString(ConfigConstants.CONFIG_CORE_SECTION,
+ null,
+ ConfigConstants.CONFIG_KEY_SUPPORTSATOMICFILECREATION);
+ if (value == null) {
+ return null;
+ }
+ return Boolean.valueOf(StringUtils.toBoolean(value));
+ } catch (IOException | ConfigInvalidException e) {
+ return Boolean.TRUE;
+ }
+ }
+
@Override
public FS newInstance() {
return new FS_POSIX(this);
@@ -301,4 +336,55 @@ public class FS_POSIX extends FS {
return hookPath.toFile();
return null;
}
+
+ @Override
+ public boolean supportsAtomicCreateNewFile() {
+ if (supportsAtomicCreateNewFile == null) {
+ determineAtomicFileCreationSupport();
+ }
+ return supportsAtomicCreateNewFile.booleanValue();
+ }
+
+ @SuppressWarnings("boxing")
+ /**
+ * An implementation of the File#createNewFile() semantics which works 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 hardlink 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.
+ *
+ * @see https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html
+ * @since 4.5
+ */
+ public boolean createNewFile(File lock) throws IOException {
+ if (!lock.createNewFile()) {
+ return false;
+ }
+ if (supportsAtomicCreateNewFile() || !supportsUnixNLink) {
+ return true;
+ }
+ Path lockPath = lock.toPath();
+ Path link = Files.createLink(Paths.get(lock.getAbsolutePath() + ".lnk"), //$NON-NLS-1$
+ lockPath);
+ try {
+ Integer nlink = (Integer) (Files.getAttribute(lockPath,
+ "unix:nlink")); //$NON-NLS-1$
+ 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;
+ }
+ return true;
+ } catch (UnsupportedOperationException | IllegalArgumentException e) {
+ supportsUnixNLink = false;
+ return true;
+ } finally {
+ Files.delete(link);
+ }
+ }
}

Back to the top