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

NOTE: port back from master branch.

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: Iaee2301eb14e6201406398a90228ad10cfea6098

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 509935dfb9e156e1446bb73d36396335b8f4edde..7eab1dcb09a50d1b561b4f105900f0aa0b53fe3b 100644 (file)
--- 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 7b5f00e4fe779b09ef89a4f1ac2e4b02c27e6983..2443c4e7712bb34e6a81505e0110683d94ca303b 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
@@ -216,9 +216,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 662aa750e610d23fdbd56d94cc5d6de536e022e7..eff2b97eabdd27b4da5b13cd18bfc36d65c6f5cd 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -48,7 +48,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;
@@ -263,8 +262,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$
@@ -286,8 +286,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$
@@ -297,6 +298,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
                 *
@@ -453,11 +466,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);
@@ -475,6 +490,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++;
@@ -541,6 +557,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();