Browse Source

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
tags/v4.5.4.201711221230-r
Christian Halstrick 6 years ago
parent
commit
10e65cb4fa

+ 2
- 2
org.eclipse.jgit/.settings/.api_filters View 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>

+ 1
- 1
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java View 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);

+ 7
- 0
org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java View 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

+ 31
- 0
org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java View 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)}.
*

+ 86
- 0
org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java View 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);
}
}
}

Loading…
Cancel
Save