From 10e65cb4faf943d4a2a782ceef6f0f09934b150b Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Tue, 14 Nov 2017 17:08:56 +0100 Subject: [PATCH] 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 --- org.eclipse.jgit/.settings/.api_filters | 4 +- .../jgit/internal/storage/file/LockFile.java | 2 +- .../org/eclipse/jgit/lib/ConfigConstants.java | 7 ++ .../src/org/eclipse/jgit/util/FS.java | 31 +++++++ .../src/org/eclipse/jgit/util/FS_POSIX.java | 86 +++++++++++++++++++ 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 @@ - + - + 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 @@ -289,6 +289,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 @@ -236,6 +236,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. @@ -776,6 +791,22 @@ public abstract class FS { FileUtils.createSymLink(path, target); } + /** + * 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 false + * + * @param path + * the file to be created + * @return true if the file was created, false 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)}. * 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); + } + } } -- 2.39.5