Skip to content

Commit

Permalink
Merge "ShutdownHook: run on bundle deactivation if in OSGi"
Browse files Browse the repository at this point in the history
  • Loading branch information
msohn authored and gerritforge-ltd committed Mar 9, 2024
2 parents 328008a + bf7dd9a commit 9917074
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 20 deletions.
2 changes: 2 additions & 0 deletions org.eclipse.jgit/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -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

This comment has been minimized.

Copy link
@laeubi

laeubi Mar 10, 2024

I think this is one of the cases where a BundleActivator would be more appropriate than a component:

  1. You want to react on bundle lifecycle not component lifecycle (this would be more appropriate if JGit would register a service
  2. If no SCR is there this will be not effective
  3. If Scr runs a bit later it might not have detected is OSGi correctly...
Eclipse-ExtensibleAPI: true
Export-Package: org.eclipse.jgit.annotations;version="6.10.0",
org.eclipse.jgit.api;version="6.10.0";
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,7 @@ public static JGitText get() {
/***/ public String shortReadOfOptionalDIRCExtensionExpectedAnotherBytes;
/***/ public String shortSkipOfBlock;
/***/ public String shutdownCleanup;
/***/ public String shutdownCleanupFailed;
/***/ public String shutdownCleanupListenerFailed;
/***/ public String signatureVerificationError;
/***/ public String signatureVerificationUnavailable;
Expand Down
Original file line number Diff line number Diff line change
@@ -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

This comment has been minimized.

Copy link
@laeubi

laeubi Mar 10, 2024

This comment is wrong, OSGi won't do that SCR will do that and this component does not register any service at all.

* 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;

This comment has been minimized.

Copy link
@laeubi

laeubi Mar 11, 2024

FrameworkUtil.getBundle(CleanupService.class) != null will tel you if you are running inside OSGi


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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@
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 {
Expand All @@ -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>
Expand All @@ -65,11 +69,7 @@ public interface Listener {
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() {
Expand All @@ -82,9 +82,7 @@ private void cleanup() {
}).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);

This comment has been minimized.

Copy link
@laeubi

laeubi Mar 10, 2024

Maybe just fetching the text in the constructor instead of in the cleanup action where it is not completely clear that it will work, beside that is this really something for an ERROR? e.g. what is the consequence if doCleanup can not run? Is it harmful? must the user perform any manual actions?

}
runner.shutdownNow();
}
Expand All @@ -104,12 +102,12 @@ private void notify(Listener l) {
}

/**
* 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
*/
Expand All @@ -123,8 +121,8 @@ public boolean register(Listener l) {
}

/**
* 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
Expand All @@ -142,9 +140,9 @@ public boolean unregister(Listener l) {
}

/**
* 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;
Expand Down

0 comments on commit 9917074

Please sign in to comment.