Remove stray files (probes or lock files) created by background threads

On process exit, it was possible that the filesystem timestamp
resolution measurement left behind .probe files or even a lock file
for the jgit.config.

Ensure the SAVE_RUNNER is shut down when the process exits (via
System.exit() or otherwise). Move lf.lock() into the try-finally
block when saving the config file.

Delete .probe files on JVM shutdown -- they are created in daemon
threads that may terminate abruptly, not executing the "finally"
clause that normally removes these files.

Bug: 579445
Change-Id: Ie27aca1fdaddfa487ebe072c3913d78355910df8

3 files changed, 39 insertions, 9 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
index 509935dfb9..7eab1dcb09 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java
@@ -200,4 +200,16 @@ public class LockFileTest extends RepositoryTestCase {
 		assertFalse(lock.isLocked());
 		checkFile(f, "contentother");
 	}
+
+	@Test
+	public void testUnlockNoop() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		try {
+			LockFile lock = new LockFile(f);
+			lock.unlock();
+			lock.unlock();
+		} catch (Throwable e) {
+			fail("unlock should be noop if not locked at all.");
+		}
+	}
 }
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 567e40936a..cba5e1697c 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
@@ -180,9 +180,10 @@ public class FileBasedConfig extends StoredConfig {
 		}
 
 		final LockFile lf = new LockFile(getFile());
-		if (!lf.lock())
-			throw new LockFailedException(getFile());
 		try {
+			if (!lf.lock()) {
+				throw new LockFailedException(getFile());
+			}
 			lf.setNeedSnapshotNoConfig(true);
 			lf.write(out);
 			if (!lf.commit())
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 9237c0a9b2..e8f38d8fd9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -47,7 +47,6 @@ import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -262,8 +261,9 @@ public abstract class FS {
 		 *
 		 * @see java.util.concurrent.Executors#newCachedThreadPool()
 		 */
-		private static final Executor FUTURE_RUNNER = new ThreadPoolExecutor(0,
-				5, 30L, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(),
+		private static final ExecutorService FUTURE_RUNNER = new ThreadPoolExecutor(
+				0, 5, 30L, TimeUnit.SECONDS,
+				new LinkedBlockingQueue<Runnable>(),
 				runnable -> {
 					Thread t = new Thread(runnable,
 							"JGit-FileStoreAttributeReader-" //$NON-NLS-1$
@@ -285,8 +285,9 @@ public abstract class FS {
 		 * small keep-alive time to avoid delays on shut-down.
 		 * </p>
 		 */
-		private static final Executor SAVE_RUNNER = new ThreadPoolExecutor(0, 1,
-				1L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>(),
+		private static final ExecutorService SAVE_RUNNER = new ThreadPoolExecutor(
+				0, 1, 1L, TimeUnit.MILLISECONDS,
+				new LinkedBlockingQueue<Runnable>(),
 				runnable -> {
 					Thread t = new Thread(runnable,
 							"JGit-FileStoreAttributeWriter-" //$NON-NLS-1$
@@ -296,6 +297,18 @@ public abstract class FS {
 					return t;
 				});
 
+		static {
+			// Shut down the SAVE_RUNNER on System.exit()
+			Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+				try {
+					SAVE_RUNNER.shutdownNow();
+					SAVE_RUNNER.awaitTermination(100, TimeUnit.MILLISECONDS);
+				} catch (Exception e) {
+					// Ignore; we're shutting down
+				}
+			}));
+		}
+
 		/**
 		 * Whether FileStore attributes should be determined asynchronously
 		 *
@@ -452,11 +465,13 @@ public abstract class FS {
 					return null;
 				}
 				// fall through and return fallback
-			} catch (IOException | InterruptedException
-					| ExecutionException | CancellationException e) {
+			} catch (IOException | ExecutionException | CancellationException e) {
 				LOG.error(e.getMessage(), e);
 			} catch (TimeoutException | SecurityException e) {
 				// use fallback
+			} catch (InterruptedException e) {
+				LOG.error(e.getMessage(), e);
+				Thread.currentThread().interrupt();
 			}
 			LOG.debug("{}: use fallback timestamp resolution for directory {}", //$NON-NLS-1$
 					Thread.currentThread(), dir);
@@ -474,6 +489,7 @@ public abstract class FS {
 			Path probe = dir.resolve(".probe-" + UUID.randomUUID()); //$NON-NLS-1$
 			Instant end = Instant.now().plusSeconds(3);
 			try {
+				probe.toFile().deleteOnExit();
 				Files.createFile(probe);
 				do {
 					n++;
@@ -540,6 +556,7 @@ public abstract class FS {
 			}
 			Path probe = dir.resolve(".probe-" + UUID.randomUUID()); //$NON-NLS-1$
 			try {
+				probe.toFile().deleteOnExit();
 				Files.createFile(probe);
 				Duration fsResolution = getFsResolution(s, dir, probe);
 				Duration clockResolution = measureClockResolution();