ShutdownHook: run on bundle deactivation if in OSGi

Running as a JVM shutdown hook is far too late in an OSGi framework; by
the time the JVM shuts down, the OSGi framework will normally already
have deactivated and unloaded bundles, and thus the JGit cleanup code
may try to work with unloaded classes for which there will be no
classloader anymore.

When JGit is used in an OSGi framework, the cleanups must run on bundle
deactivation, not on JVM shut down.

Add a declarative OSGi service CleanupService. This is a normal Java
class that has no dependencies on any OSGi bundle or interface, but
that is declared in the MANIFEST.MF and in an OSGi Service XML as an
OSGi immediate component. Set the bundle activation policy to "lazy".
(A declarative service is used instead of a bundle activator because the
latter would need to implement the OSGi interface BundleActivator, but
JGit should not have dependencies on OSGi.)

When JGit runs in an OSGi framework, the framework will create an
instance of CleanupService through the no-args constructor when (and
before) the first class from this bundle is loaded. This instance thus
knows that it is operating in OSGi, and will run the ShutdownHook when
the bundle is deactivated: bundle deactivation will deactivate the
CleanupService instance.

When JGit does not run in an OSGi framework, the OSGi service
declaration will be ignored, and there will be no already existing
CleanupService instance. We create one lazily, which thus knows that
it is not operating in OSGi, and which will use a JVM shutdown hook to
run the ShutdownHook.

This also reverts commit e6d83d61eade6dee223757d149a4df9650752a55.

Bug: jgit-36
Change-Id: I9c621b0707453c087f638974312ea1bf8ec30c31
Signed-off-by: Thomas Wolf <twolf@apache.org>

6 files changed, 118 insertions, 20 deletions

diff --git a/org.eclipse.jgit/META-INF/MANIFEST.MF b/org.eclipse.jgit/META-INF/MANIFEST.MF
index 28887948af..e897b24d0f 100644
--- a/org.eclipse.jgit/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit/META-INF/MANIFEST.MF
@@ -6,6 +6,8 @@ Bundle-SymbolicName: org.eclipse.jgit
 Bundle-Version: 6.10.0.qualifier
 Bundle-Localization: OSGI-INF/l10n/plugin
 Bundle-Vendor: %Bundle-Vendor
+Bundle-ActivationPolicy: lazy
+Service-Component: OSGI-INF/org.eclipse.jgit.internal.util.CleanupService.xml
 Eclipse-ExtensibleAPI: true
 Export-Package: org.eclipse.jgit.annotations;version="6.10.0",
  org.eclipse.jgit.api;version="6.10.0";
diff --git a/org.eclipse.jgit/OSGI-INF/org.eclipse.jgit.internal.util.CleanupService.xml b/org.eclipse.jgit/OSGI-INF/org.eclipse.jgit.internal.util.CleanupService.xml
new file mode 100644
index 0000000000..8d97374c66
--- /dev/null
+++ b/org.eclipse.jgit/OSGI-INF/org.eclipse.jgit.internal.util.CleanupService.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" activate="start" deactivate="shutDown" name="org.eclipse.jgit.internal.util.CleanupService">
+   <implementation class="org.eclipse.jgit.internal.util.CleanupService"/>
+</scr:component>
\ No newline at end of file
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index bbfd0b0d3a..19c90086aa 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -716,6 +716,7 @@ shortReadOfBlock=Short read of block.
 shortReadOfOptionalDIRCExtensionExpectedAnotherBytes=Short read of optional DIRC extension {0}; expected another {1} bytes within the section.
 shortSkipOfBlock=Short skip of block.
 shutdownCleanup=Cleanup {} during JVM shutdown
+shutdownCleanupFailed=Cleanup during JVM shutdown failed
 shutdownCleanupListenerFailed=Cleanup of {0} during JVM shutdown failed
 signatureVerificationError=Signature verification failed
 signatureVerificationUnavailable=No signature verifier registered
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index ef464e3172..700b54a7a6 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -745,6 +745,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String shortReadOfOptionalDIRCExtensionExpectedAnotherBytes;
 	/***/ public String shortSkipOfBlock;
 	/***/ public String shutdownCleanup;
+	/***/ public String shutdownCleanupFailed;
 	/***/ public String shutdownCleanupListenerFailed;
 	/***/ public String signatureVerificationError;
 	/***/ public String signatureVerificationUnavailable;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/CleanupService.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/CleanupService.java
new file mode 100644
index 0000000000..76e09307ab
--- /dev/null
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/CleanupService.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2024, Thomas Wolf <twolf@apache.org> and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+package org.eclipse.jgit.internal.util;
+
+/**
+ * A class that is registered as an OSGi service via the manifest. If JGit runs
+ * in OSGi, OSGi will instantiate a singleton as soon as the bundle is activated
+ * since this class is an immediate OSGi component with no dependencies. OSGi
+ * will then call its {@link #start()} method. If JGit is not running in OSGi,
+ * {@link #getInstance()} will lazily create an instance.
+ * <p>
+ * An OSGi-created {@link CleanupService} will run the registered cleanup when
+ * the {@code org.eclipse.jgit} bundle is deactivated. A lazily created instance
+ * will register the cleanup as a JVM shutdown hook.
+ * </p>
+ */
+public final class CleanupService {
+
+	private static final Object LOCK = new Object();
+
+	private static CleanupService INSTANCE;
+
+	private final boolean isOsgi;
+
+	private Runnable cleanup;
+
+	/**
+	 * Public component constructor for OSGi DS. Do <em>not</em> call this
+	 * explicitly! (Unfortunately this constructor must be public because of
+	 * OSGi requirements.)
+	 */
+	public CleanupService() {
+		this.isOsgi = true;
+		setInstance(this);
+	}
+
+	private CleanupService(boolean isOsgi) {
+		this.isOsgi = isOsgi;
+	}
+
+	private static void setInstance(CleanupService service) {
+		synchronized (LOCK) {
+			INSTANCE = service;
+		}
+	}
+
+	/**
+	 * Obtains the singleton instance of the {@link CleanupService} that knows
+	 * whether or not it is running on OSGi.
+	 *
+	 * @return the {@link CleanupService} singleton instance
+	 */
+	public static CleanupService getInstance() {
+		synchronized (LOCK) {
+			if (INSTANCE == null) {
+				INSTANCE = new CleanupService(false);
+			}
+			return INSTANCE;
+		}
+	}
+
+	void start() {
+		// Nothing to do
+	}
+
+	void register(Runnable cleanUp) {
+		if (isOsgi) {
+			cleanup = cleanUp;
+		} else {
+			try {
+				Runtime.getRuntime().addShutdownHook(new Thread(cleanUp));
+			} catch (IllegalStateException e) {
+				// Ignore -- the JVM is already shutting down.
+			}
+		}
+	}
+
+	void shutDown() {
+		if (isOsgi && cleanup != null) {
+			Runnable r = cleanup;
+			cleanup = null;
+			r.run();
+		}
+	}
+}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/ShutdownHook.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/ShutdownHook.java
index f52025fd6b..5ba33dbbff 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/ShutdownHook.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/ShutdownHook.java
@@ -24,8 +24,12 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * A hook registered as a JVM shutdown hook managing a set of objects needing
- * cleanup during JVM shutdown. See {@link Runtime#addShutdownHook}.
+ * The singleton {@link ShutdownHook} provides a means to register
+ * {@link Listener}s that are run when JGit is uninstalled, either
+ * <ul>
+ * <li>in an OSGi framework when this bundle is deactivated, or</li>
+ * <li>otherwise, when the JVM as a whole shuts down.</li>
+ * </ul>
  */
 @SuppressWarnings("ImmutableEnumChecker")
 public enum ShutdownHook {
@@ -35,11 +39,11 @@ public enum ShutdownHook {
 	INSTANCE;
 
 	/**
-	 * Object that needs to cleanup on JVM shutdown.
+	 * Object that needs to cleanup on shutdown.
 	 */
 	public interface Listener {
 		/**
-		 * Cleanup resources when JVM shuts down, called from JVM shutdown hook.
+		 * Cleanup resources when JGit is shut down.
 		 * <p>
 		 * Implementations should be coded defensively
 		 * <ul>
@@ -65,11 +69,7 @@ public enum ShutdownHook {
 	private volatile boolean shutdownInProgress;
 
 	private ShutdownHook() {
-		try {
-			Runtime.getRuntime().addShutdownHook(new Thread(this::cleanup));
-		} catch (IllegalStateException e) {
-			// ignore - the VM is already shutting down
-		}
+		CleanupService.getInstance().register(this::cleanup);
 	}
 
 	private void cleanup() {
@@ -82,9 +82,7 @@ public enum ShutdownHook {
 			}).get(30L, TimeUnit.SECONDS);
 		} catch (RejectedExecutionException | InterruptedException
 				| ExecutionException | TimeoutException e) {
-			// message isn't localized since during shutdown there's no
-			// guarantee which classes are still loaded
-			LOG.error("Cleanup during JVM shutdown failed", e); //$NON-NLS-1$
+			LOG.error(JGitText.get().shutdownCleanupFailed, e);
 		}
 		runner.shutdownNow();
 	}
@@ -104,12 +102,12 @@ public enum ShutdownHook {
 	}
 
 	/**
-	 * Register object that needs cleanup during JVM shutdown if it is not
-	 * already registered. Registration is disabled when JVM shutdown is already
-	 * in progress.
+	 * Register object that needs cleanup during JGit shutdown if it is not
+	 * already registered. Registration is disabled when JGit shutdown is
+	 * already in progress.
 	 *
 	 * @param l
-	 *            the object to call {@link Listener#onShutdown} on when JVM
+	 *            the object to call {@link Listener#onShutdown} on when JGit
 	 *            shuts down
 	 * @return {@code true} if this object has been registered
 	 */
@@ -123,8 +121,8 @@ public enum ShutdownHook {
 	}
 
 	/**
-	 * Unregister object that no longer needs cleanup during JVM shutdown if it
-	 * is still registered. Unregistration is disabled when JVM shutdown is
+	 * Unregister object that no longer needs cleanup during JGit shutdown if it
+	 * is still registered. Unregistration is disabled when JGit shutdown is
 	 * already in progress.
 	 *
 	 * @param l
@@ -142,9 +140,9 @@ public enum ShutdownHook {
 	}
 
 	/**
-	 * Whether a JVM shutdown is in progress
+	 * Whether a JGit shutdown is in progress
 	 *
-	 * @return {@code true} if a JVM shutdown is in progress
+	 * @return {@code true} if a JGit shutdown is in progress
 	 */
 	public boolean isShutdownInProgress() {
 		return shutdownInProgress;