Re-try reading a file when there are concurrent writes

Git and JGit are very careful to replace git files atomically when
writing. The normal mechanism for this is to write to a temporary
file and then to rename it atomically to the final destination. This
works fine on POSIX-compliant systems, but on systems where renaming
may not be atomic, exceptions may be thrown if code tries to read
the file while the rename is still ongoing. This happens in particular
on Windows, where the typical symptom is that a FileNotFoundException
with message "The process cannot access the file because it is being
used by another process" is thrown, but file.isFile() == true at the
same time.

In FileBasedConfig, a re-try was already implemented for this case.
But the same problem can also occur in other places, for instance
in RefDirectory when reading loose or packed refs. Additionally,
JGit has similar re-tries when a stale NFS file handle is detected,
but that mechanism wasn't used consistently (only for git configs
and packed refs, but not for loose refs).

Factor out the general re-try mechanism for reading into a new method
FileUtils.readWithRetry() and use that in all three places. The
re-try parameters are hardcoded: at most 5 times for stale NFS handles,
and at most 5 times with increasing backoff delays (50, 100, 200, 400,
and 800ms) for the above concurrent write case.

Bug: 579116
Change-Id: If0c2ad367446d3c0f32b509274cf8e814aca12cf
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

3 files changed, 168 insertions, 107 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
index 07e38147f7..4aa2edff38 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -28,7 +28,6 @@ import static org.eclipse.jgit.lib.Ref.Storage.PACKED;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.InterruptedIOException;
@@ -892,38 +891,27 @@ public class RefDirectory extends RefDatabase {
 	}
 
 	private PackedRefList readPackedRefs() throws IOException {
-		int maxStaleRetries = 5;
-		int retries = 0;
-		while (true) {
-			final FileSnapshot snapshot = FileSnapshot.save(packedRefsFile);
-			final MessageDigest digest = Constants.newMessageDigest();
-			try (BufferedReader br = new BufferedReader(new InputStreamReader(
-					new DigestInputStream(new FileInputStream(packedRefsFile),
-							digest),
-					UTF_8))) {
-				try {
-					return new PackedRefList(parsePackedRefs(br), snapshot,
-							ObjectId.fromRaw(digest.digest()));
-				} catch (IOException e) {
-					if (FileUtils.isStaleFileHandleInCausalChain(e)
-							&& retries < maxStaleRetries) {
-						if (LOG.isDebugEnabled()) {
-							LOG.debug(MessageFormat.format(
-									JGitText.get().packedRefsHandleIsStale,
-									Integer.valueOf(retries)), e);
+		try {
+			PackedRefList result = FileUtils.readWithRetries(packedRefsFile,
+					f -> {
+						FileSnapshot snapshot = FileSnapshot.save(f);
+						MessageDigest digest = Constants.newMessageDigest();
+						try (BufferedReader br = new BufferedReader(
+								new InputStreamReader(
+										new DigestInputStream(
+												new FileInputStream(f), digest),
+										UTF_8))) {
+							return new PackedRefList(parsePackedRefs(br),
+									snapshot,
+									ObjectId.fromRaw(digest.digest()));
 						}
-						retries++;
-						continue;
-					}
-					throw e;
-				}
-			} catch (FileNotFoundException noPackedRefs) {
-				if (packedRefsFile.exists()) {
-					throw noPackedRefs;
-				}
-				// Ignore it and leave the new list empty.
-				return NO_PACKED_REFS;
-			}
+					});
+			return result != null ? result : NO_PACKED_REFS;
+		} catch (IOException e) {
+			throw e;
+		} catch (Exception e) {
+			throw new IOException(MessageFormat
+					.format(JGitText.get().cannotReadFile, packedRefsFile), e);
 		}
 	}
 
@@ -1090,40 +1078,55 @@ public class RefDirectory extends RefDatabase {
 		}
 
 		final int limit = 4096;
-		final byte[] buf;
-		FileSnapshot otherSnapshot = FileSnapshot.save(path);
-		try {
-			buf = IO.readSome(path, limit);
-		} catch (FileNotFoundException noFile) {
-			if (path.isFile()) {
-				throw noFile;
+
+		class LooseItems {
+			final FileSnapshot snapshot;
+
+			final byte[] buf;
+
+			LooseItems(FileSnapshot snapshot, byte[] buf) {
+				this.snapshot = snapshot;
+				this.buf = buf;
 			}
-			return null; // doesn't exist or no file; not a reference.
 		}
-
-		int n = buf.length;
+		LooseItems loose = null;
+		try {
+			loose = FileUtils.readWithRetries(path,
+					f -> new LooseItems(FileSnapshot.save(f),
+							IO.readSome(f, limit)));
+		} catch (IOException e) {
+			throw e;
+		} catch (Exception e) {
+			throw new IOException(
+					MessageFormat.format(JGitText.get().cannotReadFile, path),
+					e);
+		}
+		if (loose == null) {
+			return null;
+		}
+		int n = loose.buf.length;
 		if (n == 0)
 			return null; // empty file; not a reference.
 
-		if (isSymRef(buf, n)) {
+		if (isSymRef(loose.buf, n)) {
 			if (n == limit)
 				return null; // possibly truncated ref
 
 			// trim trailing whitespace
-			while (0 < n && Character.isWhitespace(buf[n - 1]))
+			while (0 < n && Character.isWhitespace(loose.buf[n - 1]))
 				n--;
 			if (n < 6) {
-				String content = RawParseUtils.decode(buf, 0, n);
+				String content = RawParseUtils.decode(loose.buf, 0, n);
 				throw new IOException(MessageFormat.format(JGitText.get().notARef, name, content));
 			}
-			final String target = RawParseUtils.decode(buf, 5, n);
+			final String target = RawParseUtils.decode(loose.buf, 5, n);
 			if (ref != null && ref.isSymbolic()
 					&& ref.getTarget().getName().equals(target)) {
 				assert(currentSnapshot != null);
-				currentSnapshot.setClean(otherSnapshot);
+				currentSnapshot.setClean(loose.snapshot);
 				return ref;
 			}
-			return newSymbolicRef(otherSnapshot, name, target);
+			return newSymbolicRef(loose.snapshot, name, target);
 		}
 
 		if (n < OBJECT_ID_STRING_LENGTH)
@@ -1131,23 +1134,23 @@ public class RefDirectory extends RefDatabase {
 
 		final ObjectId id;
 		try {
-			id = ObjectId.fromString(buf, 0);
+			id = ObjectId.fromString(loose.buf, 0);
 			if (ref != null && !ref.isSymbolic()
 					&& id.equals(ref.getTarget().getObjectId())) {
 				assert(currentSnapshot != null);
-				currentSnapshot.setClean(otherSnapshot);
+				currentSnapshot.setClean(loose.snapshot);
 				return ref;
 			}
 
 		} catch (IllegalArgumentException notRef) {
-			while (0 < n && Character.isWhitespace(buf[n - 1]))
+			while (0 < n && Character.isWhitespace(loose.buf[n - 1]))
 				n--;
-			String content = RawParseUtils.decode(buf, 0, n);
+			String content = RawParseUtils.decode(loose.buf, 0, n);
 
 			throw new IOException(MessageFormat.format(JGitText.get().notARef,
 					name, content), notRef);
 		}
-		return new LooseUnpeeled(otherSnapshot, name, id);
+		return new LooseUnpeeled(loose.snapshot, name, id);
 	}
 
 	private static boolean isSymRef(byte[] buf, int n) {
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 7b5f00e4fe..567e40936a 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
@@ -20,7 +20,6 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.text.MessageFormat;
 
@@ -37,15 +36,11 @@ import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * The configuration file that is stored in the file of the file system.
  */
 public class FileBasedConfig extends StoredConfig {
-	private static final Logger LOG = LoggerFactory
-			.getLogger(FileBasedConfig.class);
 
 	private final File configFile;
 
@@ -115,16 +110,15 @@ public class FileBasedConfig extends StoredConfig {
 	 */
 	@Override
 	public void load() throws IOException, ConfigInvalidException {
-		final int maxRetries = 5;
-		int retryDelayMillis = 20;
-		int retries = 0;
-		while (true) {
-			final FileSnapshot oldSnapshot = snapshot;
-			final FileSnapshot newSnapshot;
-			// don't use config in this snapshot to avoid endless recursion
-			newSnapshot = FileSnapshot.saveNoConfig(getFile());
-			try {
-				final byte[] in = IO.readFully(getFile());
+		try {
+			FileSnapshot[] lastSnapshot = { null };
+			Boolean wasRead = FileUtils.readWithRetries(getFile(), f -> {
+				final FileSnapshot oldSnapshot = snapshot;
+				final FileSnapshot newSnapshot;
+				// don't use config in this snapshot to avoid endless recursion
+				newSnapshot = FileSnapshot.saveNoConfig(f);
+				lastSnapshot[0] = newSnapshot;
+				final byte[] in = IO.readFully(f);
 				final ObjectId newHash = hash(in);
 				if (hash.equals(newHash)) {
 					if (oldSnapshot.equals(newSnapshot)) {
@@ -145,47 +139,17 @@ public class FileBasedConfig extends StoredConfig {
 					snapshot = newSnapshot;
 					hash = newHash;
 				}
-				return;
-			} catch (FileNotFoundException noFile) {
-				// might be locked by another process (see exception Javadoc)
-				if (retries < maxRetries && configFile.exists()) {
-					if (LOG.isDebugEnabled()) {
-						LOG.debug(MessageFormat.format(
-								JGitText.get().configHandleMayBeLocked,
-								Integer.valueOf(retries)), noFile);
-					}
-					try {
-						Thread.sleep(retryDelayMillis);
-					} catch (InterruptedException e) {
-						Thread.currentThread().interrupt();
-					}
-					retries++;
-					retryDelayMillis *= 2; // max wait 1260 ms
-					continue;
-				}
-				if (configFile.exists()) {
-					throw noFile;
-				}
+				return Boolean.TRUE;
+			});
+			if (wasRead == null) {
 				clear();
-				snapshot = newSnapshot;
-				return;
-			} catch (IOException e) {
-				if (FileUtils.isStaleFileHandle(e)
-						&& retries < maxRetries) {
-					if (LOG.isDebugEnabled()) {
-						LOG.debug(MessageFormat.format(
-								JGitText.get().configHandleIsStale,
-								Integer.valueOf(retries)), e);
-					}
-					retries++;
-					continue;
-				}
-				throw new IOException(MessageFormat
-						.format(JGitText.get().cannotReadFile, getFile()), e);
-			} catch (ConfigInvalidException e) {
-				throw new ConfigInvalidException(MessageFormat
-						.format(JGitText.get().cannotReadFile, getFile()), e);
+				snapshot = lastSnapshot[0];
 			}
+		} catch (IOException e) {
+			throw e;
+		} catch (Exception e) {
+			throw new ConfigInvalidException(MessageFormat
+					.format(JGitText.get().cannotReadFile, getFile()), e);
 		}
 	}
 
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
index b9dd9baa61..f013e7e095 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
@@ -17,6 +17,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.io.InterruptedIOException;
 import java.nio.channels.FileChannel;
 import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.CopyOption;
@@ -655,6 +656,99 @@ public class FileUtils {
 	}
 
 	/**
+	 * Like a {@link java.util.function.Function} but throwing an
+	 * {@link Exception}.
+	 *
+	 * @param <A>
+	 *            input type
+	 * @param <B>
+	 *            output type
+	 * @since 6.2
+	 */
+	@FunctionalInterface
+	public interface IOFunction<A, B> {
+
+		/**
+		 * Performs the function.
+		 *
+		 * @param t
+		 *            input to operate on
+		 * @return the output
+		 * @throws Exception
+		 *             if a problem occurs
+		 */
+		B apply(A t) throws Exception;
+	}
+
+	private static void backOff(long delay, IOException cause)
+			throws IOException {
+		try {
+			Thread.sleep(delay);
+		} catch (InterruptedException e) {
+			IOException interruption = new InterruptedIOException();
+			interruption.initCause(e);
+			interruption.addSuppressed(cause);
+			Thread.currentThread().interrupt(); // Re-set flag
+			throw interruption;
+		}
+	}
+
+	/**
+	 * Invokes the given {@link IOFunction}, performing a limited number of
+	 * re-tries if exceptions occur that indicate either a stale NFS file handle
+	 * or that indicate that the file may be written concurrently.
+	 *
+	 * @param <T>
+	 *            result type
+	 * @param file
+	 *            to read
+	 * @param reader
+	 *            for reading the file and creating an instance of {@code T}
+	 * @return the result of the {@code reader}, or {@code null} if the file
+	 *         does not exist
+	 * @throws Exception
+	 *             if a problem occurs
+	 * @since 6.2
+	 */
+	public static <T> T readWithRetries(File file,
+			IOFunction<File, ? extends T> reader)
+			throws Exception {
+		int maxStaleRetries = 5;
+		int retries = 0;
+		long backoff = 50;
+		while (true) {
+			try {
+				try {
+					return reader.apply(file);
+				} catch (IOException e) {
+					if (FileUtils.isStaleFileHandleInCausalChain(e)
+							&& retries < maxStaleRetries) {
+						if (LOG.isDebugEnabled()) {
+							LOG.debug(MessageFormat.format(
+									JGitText.get().packedRefsHandleIsStale,
+									Integer.valueOf(retries)), e);
+						}
+						retries++;
+						continue;
+					}
+					throw e;
+				}
+			} catch (FileNotFoundException noFile) {
+				if (!file.isFile()) {
+					return null;
+				}
+				// Probably Windows and some other thread is writing the file
+				// concurrently.
+				if (backoff > 1000) {
+					throw noFile;
+				}
+				backOff(backoff, noFile);
+				backoff *= 2; // 50, 100, 200, 400, 800 ms
+			}
+		}
+	}
+
+	/**
 	 * @param file
 	 * @return {@code true} if the passed file is a symbolic link
 	 */