Use ShutdownHook to gracefully handle JVM shutdown

in all classes which already registered their own shutdown hook
- CloneCommand
- GC#PidLock
- FS#FileStoreAttributes
- LocalDiskRepositoryTestCase#Cleanup

Change-Id: I3efc1f83f3cbbf43eeeaaedcd2bee1ef31971a72

org.eclipse.jgit.junit/META-INF/MANIFEST.MF

@@ -15,6 +15,7 @@ Import-Package: org.eclipse.jgit.annotations;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.errors;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.internal.storage.file;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.internal.storage.pack;version="[6.8.0,6.9.0)",
+ org.eclipse.jgit.internal.util;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.lib;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.merge;version="[6.8.0,6.9.0)",
  org.eclipse.jgit.revwalk;version="[6.8.0,6.9.0)",

org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java

@@ -20,7 +20,6 @@
 import java.io.IOException;
 import java.io.PrintStream;
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -28,10 +27,12 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheEntry;
 import org.eclipse.jgit.internal.storage.file.FileRepository;
+import org.eclipse.jgit.internal.util.ShutdownHook;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
@@ -114,7 +115,7 @@ private String getTestName() {
 	@Before
 	public void setUp() throws Exception {
 		tmp = File.createTempFile("jgit_" + getTestName() + '_', "_tmp");
-		CleanupThread.deleteOnShutdown(tmp);
+		Cleanup.deleteOnShutdown(tmp);
 		if (!tmp.delete() || !tmp.mkdir()) {
 			throw new IOException("Cannot create " + tmp);
 		}
@@ -222,7 +223,7 @@ public void tearDown() throws Exception {
 			recursiveDelete(tmp, false, true);
 		}
 		if (tmp != null && !tmp.exists()) {
-			CleanupThread.removed(tmp);
+			Cleanup.removed(tmp);
 		}
 		SystemReader.setInstance(null);
 	}
@@ -623,29 +624,28 @@ private static HashMap<String, String> cloneEnv() {
 		return new HashMap<>(System.getenv());
 	}
 
-	private static final class CleanupThread extends Thread {
+	private static final class Cleanup {
-		private static final CleanupThread me;
+		private static final Cleanup INSTANCE = new Cleanup();
+
 		static {
-			me = new CleanupThread();
+			ShutdownHook.INSTANCE.register(() -> INSTANCE.onShutdown());
-			Runtime.getRuntime().addShutdownHook(me);
+		}
+
+		private final Set<File> toDelete = ConcurrentHashMap.newKeySet();
+
+		private Cleanup() {
+			// empty
 		}
 
 		static void deleteOnShutdown(File tmp) {
-			synchronized (me) {
+			INSTANCE.toDelete.add(tmp);
-				me.toDelete.add(tmp);
-			}
 		}
 
 		static void removed(File tmp) {
-			synchronized (me) {
+			INSTANCE.toDelete.remove(tmp);
-				me.toDelete.remove(tmp);
-			}
 		}
 
-		private final List<File> toDelete = new ArrayList<>();
+		private void onShutdown() {
-
-		@Override
-		public void run() {
 			// On windows accidentally open files or memory
 			// mapped regions may prevent files from being deleted.
 			// Suggesting a GC increases the likelihood that our

org.eclipse.jgit/META-INF/MANIFEST.MF

@@ -124,7 +124,8 @@ Export-Package: org.eclipse.jgit.annotations;version="6.8.0",
   x-friends:="org.eclipse.jgit.ssh.apache,
    org.eclipse.jgit.ssh.jsch,
    org.eclipse.jgit.test",
- org.eclipse.jgit.internal.util;version="6.8.0";x-internal:=true,
+ org.eclipse.jgit.internal.util;version="6.8.0";
+  x-friends:=" org.eclipse.jgit.junit",
  org.eclipse.jgit.lib;version="6.8.0";
   uses:="org.eclipse.jgit.transport,
    org.eclipse.jgit.util.sha1,

org.eclipse.jgit/src/org/eclipse/jgit/api/CloneCommand.java

@@ -29,6 +29,7 @@
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.internal.util.ShutdownHook;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.BranchConfig.BranchRebaseMode;
 import org.eclipse.jgit.lib.ConfigConstants;
@@ -100,6 +101,8 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 
 	private List<String> shallowExcludes = new ArrayList<>();
 
+	private ShutdownHook.Listener shutdownListener = this::cleanup;
+
 	private enum FETCH_TYPE {
 		MULTIPLE_BRANCHES, ALL_BRANCHES, MIRROR
 	}
@@ -181,12 +184,7 @@ public Git call() throws GitAPIException, InvalidRemoteException,
 		@SuppressWarnings("resource") // Closed by caller
 		Repository repository = init();
 		FetchResult fetchResult = null;
-		Thread cleanupHook = new Thread(() -> cleanup());
+		ShutdownHook.INSTANCE.register(shutdownListener);
-		try {
-			Runtime.getRuntime().addShutdownHook(cleanupHook);
-		} catch (IllegalStateException e) {
-			// ignore - the VM is already shutting down
-		}
 		try {
 			fetchResult = fetch(repository, u);
 		} catch (IOException ioe) {
@@ -210,11 +208,7 @@ public Git call() throws GitAPIException, InvalidRemoteException,
 			cleanup();
 			throw e;
 		} finally {
-			try {
+			ShutdownHook.INSTANCE.unregister(shutdownListener);
-				Runtime.getRuntime().removeShutdownHook(cleanupHook);
-			} catch (IllegalStateException e) {
-				// ignore - the VM is already shutting down
-			}
 		}
 		try {
 			checkout(repository, fetchResult);

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java

@@ -75,6 +75,7 @@
 import org.eclipse.jgit.internal.storage.commitgraph.GraphCommits;
 import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.pack.PackWriter;
+import org.eclipse.jgit.internal.util.ShutdownHook;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.CoreConfig;
@@ -1799,7 +1800,7 @@ private class PidLock implements AutoCloseable {
 
 		private FileChannel channel;
 
-		private Thread cleanupHook;
+		private ShutdownHook.Listener shutdownListener = this::close;
 
 		PidLock() {
 			pidFile = repo.getDirectory().toPath().resolve(GC_PID);
@@ -1829,12 +1830,7 @@ boolean lock() throws IOException {
 				}
 				channel.write(ByteBuffer
 						.wrap(getProcDesc().getBytes(StandardCharsets.UTF_8)));
-				try {
+				ShutdownHook.INSTANCE.register(shutdownListener);
-					Runtime.getRuntime().addShutdownHook(
-							cleanupHook = new Thread(() -> close()));
-				} catch (IllegalStateException e) {
-					// ignore - the VM is already shutting down
-				}
 			} catch (IOException | OverlappingFileLockException e) {
 				try {
 					failedToLock();
@@ -1903,13 +1899,7 @@ private String getHostName() {
 		public void close() {
 			boolean wasLocked = false;
 			try {
-				if (cleanupHook != null) {
+				ShutdownHook.INSTANCE.unregister(shutdownListener);
-					try {
-						Runtime.getRuntime().removeShutdownHook(cleanupHook);
-					} catch (IllegalStateException e) {
-						// ignore - the VM is already shutting down
-					}
-				}
 				if (lock != null && lock.isValid()) {
 					lock.release();
 					wasLocked = true;

org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java

@@ -67,6 +67,7 @@
 import org.eclipse.jgit.errors.LockFailedException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.file.FileSnapshot;
+import org.eclipse.jgit.internal.util.ShutdownHook;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
@@ -301,18 +302,16 @@ public static final class FileStoreAttributes {
 
 		static {
 			// Shut down the SAVE_RUNNER on System.exit()
+			ShutdownHook.INSTANCE
+					.register(FileStoreAttributes::shutdownSafeRunner);
+		}
+
+		private static void shutdownSafeRunner() {
 			try {
-				Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+				SAVE_RUNNER.shutdownNow();
-					try {
+				SAVE_RUNNER.awaitTermination(100, TimeUnit.MILLISECONDS);
-						SAVE_RUNNER.shutdownNow();
+			} catch (Exception e) {
-						SAVE_RUNNER.awaitTermination(100,
+				// Ignore; we're shutting down
-								TimeUnit.MILLISECONDS);
-					} catch (Exception e) {
-						// Ignore; we're shutting down
-					}
-				}));
-			} catch (IllegalStateException e) {
-				// ignore - may fail if shutdown is already in progress
 			}
 		}