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

diff --git a/org.eclipse.jgit.junit/META-INF/MANIFEST.MF b/org.eclipse.jgit.junit/META-INF/MANIFEST.MF
index 7f85f0c063bf6f09807d2392db108ef71e148677..addb737f59b097f9cf7093d4caf03336593cc583 100644 (file)
--- a/org.eclipse.jgit.junit/META-INF/MANIFEST.MF
+++ b/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)",

diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java
index f816158b1008c670b780816ddd5cd50a52ec7111..407290a39971493b3778ed3a89ba40b12e8fdd5f 100644 (file)
--- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java
+++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java
@@ -20,7 +20,6 @@ import java.io.File;
 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.List;
 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 @@ public abstract class LocalDiskRepositoryTestCase {
        @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 abstract class LocalDiskRepositoryTestCase {
                        recursiveDelete(tmp, false, true);
                }
                if (tmp != null && !tmp.exists()) {
-                       CleanupThread.removed(tmp);
+                       Cleanup.removed(tmp);
                }
                SystemReader.setInstance(null);
        }
@@ -623,29 +624,28 @@ public abstract class LocalDiskRepositoryTestCase {
                return new HashMap<>(System.getenv());
        }
 
-       private static final class CleanupThread extends Thread {
-               private static final CleanupThread me;
+       private static final class Cleanup {
+               private static final Cleanup INSTANCE = new Cleanup();
+
                static {
-                       me = new CleanupThread();
-                       Runtime.getRuntime().addShutdownHook(me);
+                       ShutdownHook.INSTANCE.register(() -> INSTANCE.onShutdown());
+               }
+
+               private final Set<File> toDelete = ConcurrentHashMap.newKeySet();
+
+               private Cleanup() {
+                       // empty
                }
 
                static void deleteOnShutdown(File tmp) {
-                       synchronized (me) {
-                               me.toDelete.add(tmp);
-                       }
+                       INSTANCE.toDelete.add(tmp);
                }
 
                static void removed(File tmp) {
-                       synchronized (me) {
-                               me.toDelete.remove(tmp);
-                       }
+                       INSTANCE.toDelete.remove(tmp);
                }
 
-               private final List<File> toDelete = new ArrayList<>();
-
-               @Override
-               public void run() {
+               private void onShutdown() {
                        // On windows accidentally open files or memory
                        // mapped regions may prevent files from being deleted.
                        // Suggesting a GC increases the likelihood that our

diff --git a/org.eclipse.jgit/META-INF/MANIFEST.MF b/org.eclipse.jgit/META-INF/MANIFEST.MF
index 96eb1887055724204ca98edbd3db5db7680f573b..a62890de431596a9b8220cf05915b7c3d5f96e39 100644 (file)
--- a/org.eclipse.jgit/META-INF/MANIFEST.MF
+++ b/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,

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CloneCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CloneCommand.java
index 107b00e274374b518588819f899e52e612f929fc..3e034f1a6a5c11796ea9f3461cfd4df6b346349c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CloneCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CloneCommand.java
@@ -29,6 +29,7 @@ import org.eclipse.jgit.dircache.DirCacheCheckout;
 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 class CloneCommand extends TransportCommand<CloneCommand, Git> {
                @SuppressWarnings("resource") // Closed by caller
                Repository repository = init();
                FetchResult fetchResult = null;
-               Thread cleanupHook = new Thread(() -> cleanup());
-               try {
-                       Runtime.getRuntime().addShutdownHook(cleanupHook);
-               } catch (IllegalStateException e) {
-                       // ignore - the VM is already shutting down
-               }
+               ShutdownHook.INSTANCE.register(shutdownListener);
                try {
                        fetchResult = fetch(repository, u);
                } catch (IOException ioe) {
@@ -210,11 +208,7 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
                        cleanup();
                        throw e;
                } finally {
-                       try {
-                               Runtime.getRuntime().removeShutdownHook(cleanupHook);
-                       } catch (IllegalStateException e) {
-                               // ignore - the VM is already shutting down
-                       }
+                       ShutdownHook.INSTANCE.unregister(shutdownListener);
                }
                try {
                        checkout(repository, fetchResult);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
index fd9e55052640e1b53dfcc737bf61bd8ed3fec33a..6933bfb3d40fb826578ca55e9b17c93e6502bbfa 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java
@@ -75,6 +75,7 @@ import org.eclipse.jgit.internal.storage.commitgraph.CommitGraphWriter;
 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 @@ public class GC {
 
                private FileChannel channel;
 
-               private Thread cleanupHook;
+               private ShutdownHook.Listener shutdownListener = this::close;
 
                PidLock() {
                        pidFile = repo.getDirectory().toPath().resolve(GC_PID);
@@ -1829,12 +1830,7 @@ public class GC {
                                }
                                channel.write(ByteBuffer
                                                .wrap(getProcDesc().getBytes(StandardCharsets.UTF_8)));
-                               try {
-                                       Runtime.getRuntime().addShutdownHook(
-                                                       cleanupHook = new Thread(() -> close()));
-                               } catch (IllegalStateException e) {
-                                       // ignore - the VM is already shutting down
-                               }
+                               ShutdownHook.INSTANCE.register(shutdownListener);
                        } catch (IOException | OverlappingFileLockException e) {
                                try {
                                        failedToLock();
@@ -1903,13 +1899,7 @@ public class GC {
                public void close() {
                        boolean wasLocked = false;
                        try {
-                               if (cleanupHook != null) {
-                                       try {
-                                               Runtime.getRuntime().removeShutdownHook(cleanupHook);
-                                       } catch (IllegalStateException e) {
-                                               // ignore - the VM is already shutting down
-                                       }
-                               }
+                               ShutdownHook.INSTANCE.unregister(shutdownListener);
                                if (lock != null && lock.isValid()) {
                                        lock.release();
                                        wasLocked = true;

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 b8fc4fc1e22b14840db2dc2b438e33fd453cd2eb..3e95de14592c350c20bd9fac7c9822d93bfbcaee 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -67,6 +67,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 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 abstract class FS {
 
                static {
                        // Shut down the SAVE_RUNNER on System.exit()
+                       ShutdownHook.INSTANCE
+                                       .register(FileStoreAttributes::shutdownSafeRunner);
+               }
+
+               private static void shutdownSafeRunner() {
                        try {
-                               Runtime.getRuntime().addShutdownHook(new Thread(() -> {
-                                       try {
-                                               SAVE_RUNNER.shutdownNow();
-                                               SAVE_RUNNER.awaitTermination(100,
-                                                               TimeUnit.MILLISECONDS);
-                                       } catch (Exception e) {
-                                               // Ignore; we're shutting down
-                                       }
-                               }));
-                       } catch (IllegalStateException e) {
-                               // ignore - may fail if shutdown is already in progress
+                               SAVE_RUNNER.shutdownNow();
+                               SAVE_RUNNER.awaitTermination(100, TimeUnit.MILLISECONDS);
+                       } catch (Exception e) {
+                               // Ignore; we're shutting down
                        }
                }