Fix concurrent read / write issue in LockFile on Windows

LockFile.commit fails if another thread concurrently reads
the base file. The problem is fixed by retrying the rename
operation if it fails.

Change-Id: I6bb76ea7f2e6e90e3ddc45f9dd4d69bd1b6fa1eb
Bug: 308506
Signed-off-by: Jens Baumgart <jens.baumgart@sap.com>

12 files changed, 83 insertions, 25 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java
index cc10fad2b4..60238c3d8b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java
@@ -67,6 +67,7 @@ import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.storage.file.LockFile;
+import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.MutableInteger;
 import org.eclipse.jgit.util.NB;
@@ -129,7 +130,7 @@ public class DirCache {
 	 *         memory).
 	 */
 	public static DirCache newInCore() {
-		return new DirCache(null);
+		return new DirCache(null, null);
 	}
 
 	/**
@@ -141,6 +142,9 @@ public class DirCache {
 	 *
 	 * @param indexLocation
 	 *            location of the index file on disk.
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 * @return a cache representing the contents of the specified index file (if
 	 *         it exists) or an empty cache if the file does not exist.
 	 * @throws IOException
@@ -149,9 +153,9 @@ public class DirCache {
 	 *             the index file is using a format or extension that this
 	 *             library does not support.
 	 */
-	public static DirCache read(final File indexLocation)
+	public static DirCache read(final File indexLocation, final FS fs)
 			throws CorruptObjectException, IOException {
-		final DirCache c = new DirCache(indexLocation);
+		final DirCache c = new DirCache(indexLocation, fs);
 		c.read();
 		return c;
 	}
@@ -161,11 +165,14 @@ public class DirCache {
 	 * <p>
 	 * The new index will be locked and then read before it is returned to the
 	 * caller. Read failures are reported as exceptions and therefore prevent
-	 * the method from returning a partially populated index.  On read failure,
+	 * the method from returning a partially populated index. On read failure,
 	 * the lock is released.
 	 *
 	 * @param indexLocation
 	 *            location of the index file on disk.
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 * @return a cache representing the contents of the specified index file (if
 	 *         it exists) or an empty cache if the file does not exist.
 	 * @throws IOException
@@ -175,9 +182,9 @@ public class DirCache {
 	 *             the index file is using a format or extension that this
 	 *             library does not support.
 	 */
-	public static DirCache lock(final File indexLocation)
+	public static DirCache lock(final File indexLocation, final FS fs)
 			throws CorruptObjectException, IOException {
-		final DirCache c = new DirCache(indexLocation);
+		final DirCache c = new DirCache(indexLocation, fs);
 		if (!c.lock())
 			throw new IOException(MessageFormat.format(JGitText.get().cannotLock, indexLocation));
 
@@ -215,6 +222,9 @@ public class DirCache {
 	/** Our active lock (if we hold it); null if we don't have it locked. */
 	private LockFile myLock;
 
+	/** file system abstraction **/
+	private final FS fs;
+
 	/**
 	 * Create a new in-core index representation.
 	 * <p>
@@ -223,9 +233,13 @@ public class DirCache {
 	 *
 	 * @param indexLocation
 	 *            location of the index file on disk.
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 */
-	public DirCache(final File indexLocation) {
+	public DirCache(final File indexLocation, final FS fs) {
 		liveFile = indexLocation;
+		this.fs = fs;
 		clear();
 	}
 
@@ -429,7 +443,7 @@ public class DirCache {
 	public boolean lock() throws IOException {
 		if (liveFile == null)
 			throw new IOException(JGitText.get().dirCacheDoesNotHaveABackingFile);
-		final LockFile tmp = new LockFile(liveFile);
+		final LockFile tmp = new LockFile(liveFile, fs);
 		if (tmp.lock()) {
 			tmp.setNeedStatInformation(true);
 			myLock = tmp;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BaseRepositoryBuilder.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BaseRepositoryBuilder.java
index 92edb0325c..410c85fd8b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BaseRepositoryBuilder.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BaseRepositoryBuilder.java
@@ -555,7 +555,7 @@ public class BaseRepositoryBuilder<B extends BaseRepositoryBuilder, R extends Re
 			// repository and not inherited from other files.
 			//
 			File path = safeFS().resolve(getGitDir(), "config");
-			FileBasedConfig cfg = new FileBasedConfig(path);
+			FileBasedConfig cfg = new FileBasedConfig(path, safeFS());
 			try {
 				cfg.load();
 			} catch (ConfigInvalidException err) {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
index a39af86577..4be1e69d33 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
@@ -930,7 +930,7 @@ public abstract class Repository {
 	 */
 	public DirCache readDirCache() throws NoWorkTreeException,
 			CorruptObjectException, IOException {
-		return DirCache.read(getIndexFile());
+		return DirCache.read(getIndexFile(), getFS());
 	}
 
 	/**
@@ -954,7 +954,7 @@ public abstract class Repository {
 	 */
 	public DirCache lockDirCache() throws NoWorkTreeException,
 			CorruptObjectException, IOException {
-		return DirCache.lock(getIndexFile());
+		return DirCache.lock(getIndexFile(), getFS());
 	}
 
 	static byte[] gitInternalSlash(byte[] bytes) {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
index 4cbb848118..8ffbe80cc2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
@@ -59,6 +59,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
 
@@ -68,15 +69,19 @@ import org.eclipse.jgit.util.RawParseUtils;
 public class FileBasedConfig extends StoredConfig {
 	private final File configFile;
 	private volatile long lastModified;
+	private final FS fs;
 
 	/**
 	 * Create a configuration with no default fallback.
 	 *
 	 * @param cfgLocation
 	 *            the location of the configuration file on the file system
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 */
-	public FileBasedConfig(File cfgLocation) {
-		this(null, cfgLocation);
+	public FileBasedConfig(File cfgLocation, FS fs) {
+		this(null, cfgLocation, fs);
 	}
 
 	/**
@@ -86,10 +91,14 @@ public class FileBasedConfig extends StoredConfig {
 	 *            the base configuration file
 	 * @param cfgLocation
 	 *            the location of the configuration file on the file system
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 */
-	public FileBasedConfig(Config base, File cfgLocation) {
+	public FileBasedConfig(Config base, File cfgLocation, FS fs) {
 		super(base);
 		configFile = cfgLocation;
+		this.fs = fs;
 	}
 
 	/** @return location of the configuration file on disk */
@@ -138,7 +147,7 @@ public class FileBasedConfig extends StoredConfig {
 	 */
 	public void save() throws IOException {
 		final byte[] out = Constants.encode(toText());
-		final LockFile lf = new LockFile(getFile());
+		final LockFile lf = new LockFile(getFile(), fs);
 		if (!lf.lock())
 			throw new IOException(MessageFormat.format(JGitText.get().cannotLockFile, getFile()));
 		try {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java
index 15aafbdaeb..69cce71cf0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java
@@ -136,7 +136,9 @@ public class FileRepository extends Repository {
 		super(options);
 
 		userConfig = SystemReader.getInstance().openUserConfig(getFS());
-		repoConfig = new FileBasedConfig(userConfig, getFS().resolve(getDirectory(), "config"));
+		repoConfig = new FileBasedConfig(userConfig, //
+				getFS().resolve(getDirectory(), "config"), //
+				getFS());
 
 		loadUserConfig();
 		loadRepoConfig();
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
index ad89a24847..e8bc3e2cfd 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
@@ -59,6 +59,7 @@ import java.text.MessageFormat;
 import org.eclipse.jgit.JGitText;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.util.FS;
 
 /**
  * Git style file locking and replacement.
@@ -94,15 +95,21 @@ public class LockFile {
 
 	private long commitLastModified;
 
+	private final FS fs;
+
 	/**
 	 * Create a new lock for any file.
 	 *
 	 * @param f
 	 *            the file that will be locked.
+	 * @param fs
+	 *            the file system abstraction which will be necessary to perform
+	 *            certain file system operations.
 	 */
-	public LockFile(final File f) {
+	public LockFile(final File f, FS fs) {
 		ref = f;
 		lck = new File(ref.getParentFile(), ref.getName() + SUFFIX);
+		this.fs = fs;
 	}
 
 	/**
@@ -393,13 +400,32 @@ public class LockFile {
 		saveStatInformation();
 		if (lck.renameTo(ref))
 			return true;
-		if (!ref.exists() || ref.delete())
+		if (!ref.exists() || deleteRef())
 			if (lck.renameTo(ref))
 				return true;
 		unlock();
 		return false;
 	}
 
+	private boolean deleteRef() {
+		if (!fs.retryFailedLockFileCommit())
+			return ref.delete();
+
+		// File deletion fails on windows if another thread is
+		// concurrently reading the same file. So try a few times.
+		//
+		for (int attempts = 0; attempts < 10; attempts++) {
+			if (ref.delete())
+				return true;
+			try {
+				Thread.sleep(100);
+			} catch (InterruptedException e) {
+				return false;
+			}
+		}
+		return false;
+	}
+
 	private void saveStatInformation() {
 		if (needStatInformation)
 			commitLastModified = lck.lastModified();
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java
index be250114c2..dd08dfbac2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java
@@ -47,21 +47,26 @@ import java.io.File;
 import java.io.IOException;
 
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.util.FS;
 
 /** Keeps track of a {@link PackFile}'s associated <code>.keep</code> file. */
 public class PackLock {
 	private final File keepFile;
+	private final FS fs;
 
 	/**
 	 * Create a new lock for a pack file.
 	 *
 	 * @param packFile
 	 *            location of the <code>pack-*.pack</code> file.
+	 * @param fs
+	 *            the filesystem abstraction used by the repository.
 	 */
-	public PackLock(final File packFile) {
+	public PackLock(final File packFile, final FS fs) {
 		final File p = packFile.getParentFile();
 		final String n = packFile.getName();
 		keepFile = new File(p, n.substring(0, n.length() - 5) + ".keep");
+		this.fs = fs;
 	}
 
 	/**
@@ -78,7 +83,7 @@ public class PackLock {
 			return false;
 		if (!msg.endsWith("\n"))
 			msg += "\n";
-		final LockFile lf = new LockFile(keepFile);
+		final LockFile lf = new LockFile(keepFile, fs);
 		if (!lf.lock())
 			return false;
 		lf.write(Constants.encode(msg));
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
index 68b0270df9..b22b14a910 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
@@ -512,7 +512,8 @@ public class RefDirectory extends RefDatabase {
 		// we don't miss an edit made externally.
 		final PackedRefList packed = getPackedRefs();
 		if (packed.contains(name)) {
-			LockFile lck = new LockFile(packedRefsFile);
+			LockFile lck = new LockFile(packedRefsFile,
+					update.getRepository().getFS());
 			if (!lck.lock())
 				throw new IOException(MessageFormat.format(
 					JGitText.get().cannotLockFile, packedRefsFile));
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryUpdate.java
index 8d35ec34f6..a9f054837b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryUpdate.java
@@ -79,7 +79,7 @@ class RefDirectoryUpdate extends RefUpdate {
 		if (deref)
 			dst = dst.getLeaf();
 		String name = dst.getName();
-		lock = new LockFile(database.fileFor(name));
+		lock = new LockFile(database.fileFor(name), getRepository().getFS());
 		if (lock.lock()) {
 			dst = database.getRef(name);
 			setOldObjectId(dst != null ? dst.getObjectId() : null);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
index ca68858059..f747616749 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
@@ -279,7 +279,8 @@ class FetchProcess {
 		File meta = transport.local.getDirectory();
 		if (meta == null)
 			return;
-		final LockFile lock = new LockFile(new File(meta, "FETCH_HEAD"));
+		final LockFile lock = new LockFile(new File(meta, "FETCH_HEAD"),
+				transport.local.getFS());
 		try {
 			if (lock.lock()) {
 				final Writer w = new OutputStreamWriter(lock.getOutputStream());
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
index 25b499b32e..2daa105c53 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
@@ -1101,7 +1101,7 @@ public class IndexPack {
 		final File packDir = new File(repo.getObjectsDirectory(), "pack");
 		final File finalPack = new File(packDir, "pack-" + name + ".pack");
 		final File finalIdx = new File(packDir, "pack-" + name + ".idx");
-		final PackLock keep = new PackLock(finalPack);
+		final PackLock keep = new PackLock(finalPack, repo.getFS());
 
 		if (!packDir.exists() && !packDir.mkdir() && !packDir.exists()) {
 			// The objects/pack directory isn't present, and we are unable
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java
index f4382eb186..475c871c3c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/SystemReader.java
@@ -74,7 +74,7 @@ public abstract class SystemReader {
 
 		public FileBasedConfig openUserConfig(FS fs) {
 			final File home = fs.userHome();
-			return new FileBasedConfig(new File(home, ".gitconfig"));
+			return new FileBasedConfig(new File(home, ".gitconfig"), fs);
 		}
 
 		public String getHostname() {