]> source.dussan.org Git - jgit.git/commitdiff
Fix LockFile semantics when running on NFS 16/112016/9
authorChristian Halstrick <christian.halstrick@sap.com>
Tue, 14 Nov 2017 16:08:56 +0000 (17:08 +0100)
committerChristian Halstrick <christian.halstrick@sap.com>
Wed, 22 Nov 2017 17:15:11 +0000 (18:15 +0100)
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
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java
org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java
org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java

index 2c27b051b0e6cd56d38878b3cba2a0c7aa18c626..38305634959fc994257c74ec2e8aadc509d35cf1 100644 (file)
@@ -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>
index ce9677a62da09496512d60c56ccf4f02a35766fd..51af67e0bd8e235521dc3c3445704ca3ca0bdeba 100644 (file)
@@ -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);
index 9e3e0b78fde45b912235526ab030b9294423407a..e3f8ba5b5b4f2bf55086df85788d359d3ed4bbc6 100644 (file)
@@ -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
index a55b5c0ac5a61ee566ba896160959dd1dd411f5d..e1fd1cb8899a382c7a03a0cfb4e370eccdd91648 100644 (file)
@@ -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 <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)}.
         *
index cb4868cc7aff1e45d2c99099868c47b6dc8e881c..4ecaf9c8eeed15cff244bcf382ba9ff3a7933781 100644 (file)
@@ -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);
+               }
+       }
 }