You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by ck...@apache.org on 2020/11/03 14:57:52 UTC
[logging-log4j2] branch release-2.x updated: LOG4J2-2954 Retain
strong reference to shutdown callbacks
This is an automated email from the ASF dual-hosted git repository.
ckozak pushed a commit to branch release-2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/release-2.x by this push:
new a3e31f7 LOG4J2-2954 Retain strong reference to shutdown callbacks
a3e31f7 is described below
commit a3e31f76dd13a5e3da8584fc75a4380da206454a
Author: Henry Tung <ht...@palantir.com>
AuthorDate: Thu Oct 29 19:38:10 2020 -0700
LOG4J2-2954 Retain strong reference to shutdown callbacks
Make sure the returned Cancellable handler contains a strong
reference to the callback, so that it is not garbage-collected
prematurely. Add Javadoc note about collection semantics.
---
.../core/appender/mom/jeromq/JeroMqManager.java | 13 +++---
.../core/util/DefaultShutdownCallbackRegistry.java | 52 +++++++++++++---------
.../log4j/core/util/ShutdownCallbackRegistry.java | 4 ++
src/changes/changes.xml | 3 ++
4 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/mom/jeromq/JeroMqManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/mom/jeromq/JeroMqManager.java
index a438faf..327d9b1 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/mom/jeromq/JeroMqManager.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/mom/jeromq/JeroMqManager.java
@@ -24,6 +24,7 @@ import java.util.concurrent.TimeUnit;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.appender.AbstractManager;
import org.apache.logging.log4j.core.appender.ManagerFactory;
+import org.apache.logging.log4j.core.util.Cancellable;
import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry;
import org.apache.logging.log4j.util.PropertiesUtil;
import org.zeromq.ZMQ;
@@ -48,6 +49,9 @@ public class JeroMqManager extends AbstractManager {
private static final JeroMqManagerFactory FACTORY = new JeroMqManagerFactory();
private static final ZMQ.Context CONTEXT;
+ // Retained to avoid garbage collection of the hook
+ private static final Cancellable SHUTDOWN_HOOK;
+
static {
LOGGER.trace("JeroMqManager using ZMQ version {}", ZMQ.getVersionString());
@@ -58,12 +62,9 @@ public class JeroMqManager extends AbstractManager {
final boolean enableShutdownHook = PropertiesUtil.getProperties().getBooleanProperty(
SYS_PROPERTY_ENABLE_SHUTDOWN_HOOK, true);
if (enableShutdownHook) {
- ((ShutdownCallbackRegistry) LogManager.getFactory()).addShutdownCallback(new Runnable() {
- @Override
- public void run() {
- CONTEXT.close();
- }
- });
+ SHUTDOWN_HOOK = ((ShutdownCallbackRegistry) LogManager.getFactory()).addShutdownCallback(CONTEXT::close);
+ } else {
+ SHUTDOWN_HOOK = null;
}
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java
index 3c4f6e6..8313f95 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/DefaultShutdownCallbackRegistry.java
@@ -44,7 +44,9 @@ public class DefaultShutdownCallbackRegistry implements ShutdownCallbackRegistry
private final AtomicReference<State> state = new AtomicReference<>(State.INITIALIZED);
private final ThreadFactory threadFactory;
- private final Collection<Cancellable> hooks = new CopyOnWriteArrayList<>();
+
+ // use references to prevent memory leaks
+ private final Collection<Reference<Cancellable>> hooks = new CopyOnWriteArrayList<>();
private Reference<Thread> shutdownHookRef;
/**
@@ -69,15 +71,18 @@ public class DefaultShutdownCallbackRegistry implements ShutdownCallbackRegistry
@Override
public void run() {
if (state.compareAndSet(State.STARTED, State.STOPPING)) {
- for (final Runnable hook : hooks) {
- try {
- hook.run();
- } catch (final Throwable t1) {
+ for (final Reference<Cancellable> hookRef : hooks) {
+ Cancellable hook = hookRef.get();
+ if (hook != null) {
try {
- LOGGER.error(SHUTDOWN_HOOK_MARKER, "Caught exception executing shutdown hook {}", hook, t1);
- } catch (final Throwable t2) {
- System.err.println("Caught exception " + t2.getClass() + " logging exception " + t1.getClass());
- t1.printStackTrace();
+ hook.run();
+ } catch (final Throwable t1) {
+ try {
+ LOGGER.error(SHUTDOWN_HOOK_MARKER, "Caught exception executing shutdown hook {}", hook, t1);
+ } catch (final Throwable t2) {
+ System.err.println("Caught exception " + t2.getClass() + " logging exception " + t1.getClass());
+ t1.printStackTrace();
+ }
}
}
}
@@ -86,34 +91,39 @@ public class DefaultShutdownCallbackRegistry implements ShutdownCallbackRegistry
}
private static class RegisteredCancellable implements Cancellable {
- // use a reference to prevent memory leaks
- private final Reference<Runnable> hook;
- private Collection<Cancellable> registered;
+ private Runnable callback;
+ private Collection<Reference<Cancellable>> registered;
- RegisteredCancellable(final Runnable callback, final Collection<Cancellable> registered) {
+ RegisteredCancellable(final Runnable callback, final Collection<Reference<Cancellable>> registered) {
+ this.callback = callback;
this.registered = registered;
- hook = new SoftReference<>(callback);
}
@Override
public void cancel() {
- hook.clear();
- registered.remove(this);
- registered = null;
+ callback = null;
+ Collection<Reference<Cancellable>> references = registered;
+ if (references != null) {
+ registered = null;
+ references.removeIf(ref -> {
+ Cancellable value = ref.get();
+ return value == null || value == RegisteredCancellable.this;
+ });
+ }
}
@Override
public void run() {
- final Runnable runnableHook = this.hook.get();
+ final Runnable runnableHook = callback;
if (runnableHook != null) {
runnableHook.run();
- this.hook.clear();
+ callback = null;
}
}
@Override
public String toString() {
- return String.valueOf(hook.get());
+ return String.valueOf(callback);
}
}
@@ -121,7 +131,7 @@ public class DefaultShutdownCallbackRegistry implements ShutdownCallbackRegistry
public Cancellable addShutdownCallback(final Runnable callback) {
if (isStarted()) {
final Cancellable receipt = new RegisteredCancellable(callback, hooks);
- hooks.add(receipt);
+ hooks.add(new SoftReference<>(receipt));
return receipt;
}
throw new IllegalStateException("Cannot add new shutdown hook as this is not started. Current state: " +
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistry.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistry.java
index 68478dd..a7b703e 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistry.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistry.java
@@ -47,6 +47,10 @@ public interface ShutdownCallbackRegistry {
/**
* Adds a Runnable shutdown callback to this class.
*
+ * Note: The returned {@code Cancellable} must be retained on heap by caller
+ * to avoid premature garbage-collection of the registered callback (and to ensure
+ * the callback runs on shutdown).
+ *
* @param callback the shutdown callback to be executed upon shutdown.
* @return a Cancellable wrapper of the provided callback or {@code null} if the shutdown hook is disabled and
* cannot be added.
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index f0f309b..9268ab4 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -153,6 +153,9 @@
<action issue="LOG4J2-2939" dev="ckozak" type="fix" due-to="Constantin Hirsch">
Fix NPE in MDCContextMap on 'contains' and 'isEmpty' invocations.
</action>
+ <action issue="LOG4J2-2954" dev="ckozak" type="fix" due-to="Henry Tung">
+ Prevent premature garbage collection of shutdown hooks in DefaultShutdownCallbackRegistry.
+ </action>
</release>
<release version="2.13.3" date="2020-05-10" description="GA Release 2.13.3">
<action issue="LOG4J2-2838" dev="rgoers" type="fix">