From d67ac798f10254d038c49244e7f1e2323afdfdfc Mon Sep 17 00:00:00 2001 From: "James Z.M. Gao" Date: Fri, 8 Apr 2022 00:29:39 +0800 Subject: [PATCH] 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 --- .../internal/storage/file/LockFileTest.java | 12 +++++++ .../jgit/storage/file/FileBasedConfig.java | 5 +-- .../src/org/eclipse/jgit/util/FS.java | 31 ++++++++++++++----- 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 7b5f00e4fe..2443c4e771 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 @@ -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 662aa750e6..eff2b97eab 100644 --- 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(), + private static final ExecutorService FUTURE_RUNNER = new ThreadPoolExecutor( + 0, 5, 30L, TimeUnit.SECONDS, + new LinkedBlockingQueue(), 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. *

*/ - private static final Executor SAVE_RUNNER = new ThreadPoolExecutor(0, 1, - 1L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue(), + private static final ExecutorService SAVE_RUNNER = new ThreadPoolExecutor( + 0, 1, 1L, TimeUnit.MILLISECONDS, + new LinkedBlockingQueue(), 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(); -- 2.39.5