Author: mattsicker Date: Sun Mar 23 17:12:35 2014 New Revision: 1580542 URL: http://svn.apache.org/r1580542 Log: Fix up circular references in LoggerContext.
- This doesn't fix LOG4J2-570, but it does help in related memory leaks. - Using a PhantomReference to hold the shutdown thread (which can be enqueue()'d upon stopping). - No need to call Runtime.getRuntime().removeShutdownHook() when it's already running! - Converted shutdown hook to a static inner class implementing Runnable (as per the recommended practice of not extending Thread). - Also cleaned up some logging statements here. Modified: logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java Modified: logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java URL: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java?rev=1580542&r1=1580541&r2=1580542&view=diff ============================================================================== --- logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java (original) +++ logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java Sun Mar 23 17:12:35 2014 @@ -19,6 +19,9 @@ package org.apache.logging.log4j.core; import java.beans.PropertyChangeEvent; import java.beans.PropertyChangeListener; import java.io.File; +import java.lang.ref.PhantomReference; +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; import java.net.URI; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -27,6 +30,8 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Marker; +import org.apache.logging.log4j.MarkerManager; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationFactory; import org.apache.logging.log4j.core.config.ConfigurationListener; @@ -51,6 +56,7 @@ public class LoggerContext implements or public static final String PROPERTY_CONFIG = "config"; private static final StatusLogger LOGGER = StatusLogger.getLogger(); + private static final Marker SHUTDOWN_HOOK = MarkerManager.getMarker("SHUTDOWN HOOK"); private static final Configuration NULL_CONFIGURATION = new NullConfiguration(); private final ConcurrentMap<String, Logger> loggers = new ConcurrentHashMap<String, Logger>(); @@ -65,7 +71,12 @@ public class LoggerContext implements or private final String name; private URI configLocation; - private ShutdownThread shutdownThread = null; + /** + * Once a shutdown hook thread executes, we can't remove it from the runtime (throws an exception). Therefore, + * it's really pointless to keep a strongly accessible reference to said thread. Thus, please use a + * PhantomReference here to prevent possible cyclic memory references. + */ + private Reference<Thread> shutdownThread; /** * Status of the LoggerContext. @@ -147,18 +158,7 @@ public class LoggerContext implements or if (status == Status.INITIALIZED || status == Status.STOPPED) { status = Status.STARTING; reconfigure(); - if (config.isShutdownHookEnabled()) { - shutdownThread = new ShutdownThread(this); - try { - Runtime.getRuntime().addShutdownHook(shutdownThread); - } catch (final IllegalStateException ise) { - LOGGER.warn("Unable to register shutdown hook due to JVM state"); - shutdownThread = null; - } catch (final SecurityException se) { - LOGGER.warn("Unable to register shutdown hook due to security restrictions"); - shutdownThread = null; - } - } + registerShutdownHookIfEnabled(); status = Status.STARTED; } } finally { @@ -174,17 +174,8 @@ public class LoggerContext implements or public void start(final Configuration config) { if (configLock.tryLock()) { try { - if ((status == Status.INITIALIZED || status == Status.STOPPED) && config.isShutdownHookEnabled() ) { - shutdownThread = new ShutdownThread(this); - try { - Runtime.getRuntime().addShutdownHook(shutdownThread); - } catch (final IllegalStateException ise) { - LOGGER.warn("Unable to register shutdown hook due to JVM state"); - shutdownThread = null; - } catch (final SecurityException se) { - LOGGER.warn("Unable to register shutdown hook due to security restrictions"); - shutdownThread = null; - } + if (status == Status.INITIALIZED || status == Status.STOPPED) { + registerShutdownHookIfEnabled(); status = Status.STARTED; } } finally { @@ -194,6 +185,21 @@ public class LoggerContext implements or setConfiguration(config); } + private void registerShutdownHookIfEnabled() { + if (config.isShutdownHookEnabled()) { + LOGGER.debug(SHUTDOWN_HOOK, "Shutdown hook enabled. Registering a new one."); + final Thread thread = new Thread(new ShutdownThread(this), "log4j-shutdown"); + shutdownThread = new PhantomReference<Thread>(thread, new ReferenceQueue<Thread>()); + try { + Runtime.getRuntime().addShutdownHook(thread); + } catch (final IllegalStateException ise) { + LOGGER.warn(SHUTDOWN_HOOK, "Unable to register shutdown hook due to JVM state"); + } catch (final SecurityException se) { + LOGGER.warn(SHUTDOWN_HOOK, "Unable to register shutdown hook due to security restrictions"); + } + } + } + @Override public void stop() { configLock.lock(); @@ -202,10 +208,7 @@ public class LoggerContext implements or return; } status = Status.STOPPING; - if (shutdownThread != null) { - Runtime.getRuntime().removeShutdownHook(shutdownThread); - shutdownThread = null; - } + unregisterShutdownHook(); final Configuration prev = config; config = NULL_CONFIGURATION; updateLoggers(); @@ -215,12 +218,19 @@ public class LoggerContext implements or status = Status.STOPPED; } finally { configLock.unlock(); - - // in finally: unregister MBeans even if an exception occurred while stopping + + // in finally: unregister MBeans even if an exception occurred while stopping Server.unregisterLoggerContext(getName()); // LOG4J2-406, LOG4J2-500 } } + private void unregisterShutdownHook() { + if (shutdownThread != null) { + LOGGER.debug(SHUTDOWN_HOOK, "Enqueue shutdown hook for garbage collection."); + shutdownThread.enqueue(); + } + } + /** * Gets the name. * @@ -346,11 +356,7 @@ public class LoggerContext implements or prev.stop(); } - // notify listeners - final PropertyChangeEvent evt = new PropertyChangeEvent(this, PROPERTY_CONFIG, prev, config); - for (final PropertyChangeListener listener : propertyChangeListeners) { - listener.propertyChange(evt); - } + firePropertyChangeEvent(new PropertyChangeEvent(this, PROPERTY_CONFIG, prev, config)); try { Server.reregisterMBeansAfterReconfigure(); @@ -360,6 +366,12 @@ public class LoggerContext implements or return prev; } + private void firePropertyChangeEvent(final PropertyChangeEvent event) { + for (final PropertyChangeListener listener : propertyChangeListeners) { + listener.propertyChange(event); + } + } + public void addPropertyChangeListener(final PropertyChangeListener listener) { propertyChangeListeners.add(Assert.isNotNull(listener, "listener")); } @@ -381,7 +393,7 @@ public class LoggerContext implements or * Reconfigure the context. */ public synchronized void reconfigure() { - LOGGER.debug("Reconfiguration started for context " + name); + LOGGER.debug("Reconfiguration started for context {}", name); final Configuration instance = ConfigurationFactory.getInstance().getConfiguration(name, configLocation); setConfiguration(instance); /* @@ -417,7 +429,7 @@ public class LoggerContext implements or */ @Override public synchronized void onChange(final Reconfigurable reconfigurable) { - LOGGER.debug("Reconfiguration started for context " + name); + LOGGER.debug("Reconfiguration started for context {}", name); final Configuration config = reconfigurable.reconfigure(); if (config != null) { setConfiguration(config); @@ -432,7 +444,7 @@ public class LoggerContext implements or return new Logger(ctx, name, messageFactory); } - private class ShutdownThread extends Thread { + private static class ShutdownThread implements Runnable { private final LoggerContext context; @@ -442,8 +454,9 @@ public class LoggerContext implements or @Override public void run() { - context.shutdownThread = null; + LOGGER.debug("Stopping LoggerContext [{}]", context); context.stop(); + LOGGER.debug("LoggerContext [{}] stopped.", context); } }