From bf7dd9add2f7115cb988d582da7e8943c3f5fbbf Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 8 Mar 2024 19:48:27 +0100 Subject: [PATCH] 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 --- org.eclipse.jgit/META-INF/MANIFEST.MF | 2 + ...ipse.jgit.internal.util.CleanupService.xml | 4 + .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../jgit/internal/util/CleanupService.java | 92 +++++++++++++++++++ .../jgit/internal/util/ShutdownHook.java | 38 ++++---- 6 files changed, 118 insertions(+), 20 deletions(-) create mode 100644 org.eclipse.jgit/OSGI-INF/org.eclipse.jgit.internal.util.CleanupService.xml create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/internal/util/CleanupService.java 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 @@ + + + + \ 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 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. + *

+ * 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. + *

+ */ +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 not 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 + *
    + *
  • in an OSGi framework when this bundle is deactivated, or
    • + *
  • otherwise, when the JVM as a whole shuts down.
    • + *
*/ @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. *

* Implementations should be coded defensively *

    @@ -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; -- 2.39.5