Author: kwall Date: Tue Sep 4 13:18:32 2012 New Revision: 1380625 URL: http://svn.apache.org/viewvc?rev=1380625&view=rev Log: QPID-4271: improve behaviour when embedding the broker inside a container
Avoid potential ThreadLocal leaks on Container owned threads for CurrentActor, AMQShortString and SecurityManager. Have LogRecorder unregistered itself from Log4J. Allow SIGHUP handling to be turned off (inappropiate to install signal handling when deployed inside Container. Allow use of custom RMI socket factory to be disabled. (The registration of a custom RMI socket with the JRE cannot be reversed (deficiency in JRE API) and this causes a large perm-gen leak). Work of Robbie Gemmell <[email protected]> and myself. Modified: qpid/trunk/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java qpid/trunk/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java qpid/trunk/qpid/java/broker/etc/log4j.xml qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/Broker.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/Log4jMessageLogger.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/LogRecorder.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/actors/CurrentActor.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java Modified: qpid/trunk/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java (original) +++ qpid/trunk/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java Tue Sep 4 13:18:32 2012 @@ -36,6 +36,7 @@ import javax.servlet.http.HttpServletRes import javax.servlet.http.HttpSession; import org.apache.commons.codec.binary.Base64; import org.apache.log4j.Logger; +import org.apache.qpid.framing.AMQShortString; import org.apache.qpid.server.logging.LogActor; import org.apache.qpid.server.logging.RootMessageLogger; import org.apache.qpid.server.logging.actors.CurrentActor; @@ -218,7 +219,14 @@ public abstract class AbstractServlet ex } finally { - org.apache.qpid.server.security.SecurityManager.setThreadSubject(null); + try + { + org.apache.qpid.server.security.SecurityManager.setThreadSubject(null); + } + finally + { + AMQShortString.clearLocalCache(); + } } } Modified: qpid/trunk/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java (original) +++ qpid/trunk/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java Tue Sep 4 13:18:32 2012 @@ -211,10 +211,12 @@ public class JMXManagedObjectRegistry im System.setProperty("java.rmi.server.randomIDs", "true"); if(_useCustomSocketFactory) { + _log.debug("Using custom RMIServerSocketFactory"); _rmiRegistry = LocateRegistry.createRegistry(_jmxPortRegistryServer, null, new CustomRMIServerSocketFactory()); } else { + _log.debug("Using default RMIServerSocketFactory"); _rmiRegistry = LocateRegistry.createRegistry(_jmxPortRegistryServer, null, null); } @@ -235,7 +237,7 @@ public class JMXManagedObjectRegistry im /** * Override makeClient so we can cache the username of the client in a Map keyed by connectionId. - * ConnectionId is guaranteed to be unique per client connection, according to the JMS spec. + * ConnectionId is guaranteed to be unique per client connection, according to the JMX spec. * An instance of NotificationListener (mapCleanupListener) will be responsible for removing these Map * entries. * Modified: qpid/trunk/qpid/java/broker/etc/log4j.xml URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/etc/log4j.xml?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/etc/log4j.xml (original) +++ qpid/trunk/qpid/java/broker/etc/log4j.xml Tue Sep 4 13:18:32 2012 @@ -88,9 +88,9 @@ </appender> <!-- Provide warnings to standard output --> - <category additivity="true" name="org.apache.qpid"> - <priority value="warn"/> - </category> + <logger additivity="true" name="org.apache.qpid"> + <level value="warn"/> + </logger> <!-- Enable info messages for the status-logging hierarchy --> <logger additivity="true" name="qpid.message"> @@ -108,21 +108,14 @@ <level value="info"/> </logger> - <!-- Examples of additional logging settings --> - <!-- Used to generate extra debug. See debug.log4j.xml --> - - <!--<category additivity="true" name="org.apache.qpid.server.store"> - <priority value="debug"/> - </category--> - <!-- Set the commons logging that the XML parser uses to WARN, it is very chatty at debug --> <logger name="org.apache.commons"> - <level value="WARN"/> + <level value="warn"/> </logger> <!-- Log all info events to file --> <root> - <priority value="info"/> + <level value="info"/> <appender-ref ref="FileAppender"/> <!--appender-ref ref="ArchivingFileAppender"/--> </root> Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/Broker.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/Broker.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/Broker.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/Broker.java Tue Sep 4 13:18:32 2012 @@ -29,6 +29,7 @@ import java.util.*; import javax.net.ssl.SSLContext; import org.apache.log4j.Logger; import org.apache.log4j.PropertyConfigurator; +import org.apache.qpid.framing.AMQShortString; import org.apache.qpid.server.configuration.ServerConfiguration; import org.apache.qpid.server.configuration.ServerNetworkTransportConfiguration; import org.apache.qpid.server.logging.SystemOutMessageLogger; @@ -73,7 +74,14 @@ public class Broker } finally { - ApplicationRegistry.remove(); + try + { + ApplicationRegistry.remove(); + } + finally + { + clearAMQShortStringCache(); + } } } @@ -84,15 +92,22 @@ public class Broker public void startup(final BrokerOptions options) throws Exception { + CurrentActor.set(new BrokerActor(new SystemOutMessageLogger())); try { - CurrentActor.set(new BrokerActor(new SystemOutMessageLogger())); startupImpl(options); addShutdownHook(); } finally { - CurrentActor.remove(); + try + { + CurrentActor.remove(); + } + finally + { + clearAMQShortStringCache(); + } } } @@ -368,7 +383,7 @@ public class Broker if (!configFile.exists() && throwOnFileNotFound) { - String error = "File " + fileName + " could not be found. Check the file exists and is readable."; + String error = "File " + configFile + " could not be found. Check the file exists and is readable."; if (qpidHome == null) { @@ -540,4 +555,14 @@ public class Broker Broker.this.shutdown(); } } + + /** + * Workaround that prevents AMQShortStrings cache from being left in the thread local. This is important + * when embedding the Broker in containers where the starting thread may not belong to Qpid. + * The long term solution here is to stop our use of AMQShortString outside the AMQP transport layer. + */ + private void clearAMQShortStringCache() + { + AMQShortString.clearLocalCache(); + } } Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/ServerConfiguration.java Tue Sep 4 13:18:32 2012 @@ -67,6 +67,8 @@ public class ServerConfiguration extends public static final int DEFAULT_HTTP_MANAGEMENT_PORT = 8080; public static final int DEFAULT_HTTPS_MANAGEMENT_PORT = 8443; public static final long DEFAULT_MINIMUM_ALERT_REPEAT_GAP = 30000l; + public static final String SKIP_SIGHUP_HANDLER_REGISTRATION = "qpid.skip_sighup_handler_registration"; + public static final String USE_CUSTOM_RMI_SOCKET_FACTORY = "qpid.use_custom_rmi_socket_factory"; public static final String QPID_HOME = "QPID_HOME"; public static final String QPID_WORK = "QPID_WORK"; @@ -156,6 +158,18 @@ public class ServerConfiguration extends this(parseConfig(configurationURL)); _configFile = configurationURL; + if(!Boolean.getBoolean(SKIP_SIGHUP_HANDLER_REGISTRATION)) + { + registerSigHupHandler(); + } + else + { + _logger.info("Skipping registration of Signal HUP handler."); + } + } + + private void registerSigHupHandler() + { SignalHandlerTask hupReparseTask = new SignalHandlerTask() { public void handle() @@ -562,7 +576,8 @@ public class ServerConfiguration extends public boolean getUseCustomRMISocketFactory() { - return getBooleanValue(MGMT_CUSTOM_REGISTRY_SOCKET, true); + return getBooleanValue(MGMT_CUSTOM_REGISTRY_SOCKET, + Boolean.parseBoolean(System.getProperty(USE_CUSTOM_RMI_SOCKET_FACTORY, "true"))); } public void setUseCustomRMISocketFactory(boolean bool) Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/Log4jMessageLogger.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/Log4jMessageLogger.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/Log4jMessageLogger.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/Log4jMessageLogger.java Tue Sep 4 13:18:32 2012 @@ -20,15 +20,11 @@ */ package org.apache.qpid.server.logging; -import org.apache.log4j.Level; import org.apache.log4j.Logger; - import org.apache.qpid.server.configuration.ServerConfiguration; public class Log4jMessageLogger extends AbstractRootMessageLogger { - public static final Level LEVEL = Level.toLevel("INFO"); - public Log4jMessageLogger() { super(); @@ -51,7 +47,7 @@ public class Log4jMessageLogger extends if(isEnabled()) { Logger logger = Logger.getLogger(logHierarchy); - return logger.isEnabledFor(LEVEL); + return logger.isInfoEnabled(); } else { @@ -69,7 +65,6 @@ public class Log4jMessageLogger extends public void rawMessage(String message, Throwable throwable, String logHierarchy) { Logger logger = Logger.getLogger(logHierarchy); - - logger.log(LEVEL, message, throwable); + logger.info(message, throwable); } } Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/LogRecorder.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/LogRecorder.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/LogRecorder.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/LogRecorder.java Tue Sep 4 13:18:32 2012 @@ -90,7 +90,6 @@ public class LogRecorder implements Appe public LogRecorder() { - Logger.getRootLogger().addAppender(this); } @@ -109,7 +108,11 @@ public class LogRecorder implements Appe @Override public void close() { - //TODO - Implement + } + + public void closeLogRecorder() + { + Logger.getRootLogger().removeAppender(this); } @Override Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/actors/CurrentActor.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/actors/CurrentActor.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/actors/CurrentActor.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/logging/actors/CurrentActor.java Tue Sep 4 13:18:32 2012 @@ -105,6 +105,11 @@ public class CurrentActor { Stack<LogActor> stack = _currentActor.get(); stack.pop(); + + if (stack.isEmpty()) + { + _currentActor.remove(); + } } /** Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java Tue Sep 4 13:18:32 2012 @@ -479,6 +479,8 @@ public abstract class ApplicationRegistr close(_pluginManager); CurrentActor.get().message(BrokerMessages.STOPPED()); + + _logRecorder.closeLogRecorder(); } finally { Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java (original) +++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/security/SecurityManager.java Tue Sep 4 13:18:32 2012 @@ -69,13 +69,8 @@ public class SecurityManager /** Container for the {@link java.security.Principal} that is using to this thread. */ private static final ThreadLocal<Subject> _subject = new ThreadLocal<Subject>(); - private static final ThreadLocal<Boolean> _accessChecksDisabled = new ThreadLocal<Boolean>() - { - protected Boolean initialValue() - { - return false; - } - }; + + public static final ThreadLocal<Boolean> _accessChecksDisabled = new ClearingThreadLocal(false); private PluginManager _pluginManager; private Map<String, SecurityPluginFactory> _pluginFactories = new HashMap<String, SecurityPluginFactory>(); @@ -114,6 +109,50 @@ public class SecurityManager } } + /** + * A special ThreadLocal, which calls remove() on itself whenever the value is + * the default, to avoid leaving a default value set after its use has passed. + */ + private static final class ClearingThreadLocal extends ThreadLocal<Boolean> + { + private Boolean _defaultValue; + + public ClearingThreadLocal(Boolean defaultValue) + { + super(); + _defaultValue = defaultValue; + } + + @Override + protected Boolean initialValue() + { + return _defaultValue; + } + + @Override + public void set(Boolean value) + { + if (value == _defaultValue) + { + super.remove(); + } + else + { + super.set(value); + } + } + + @Override + public Boolean get() + { + Boolean value = super.get(); + if (value == _defaultValue) + { + super.remove(); + } + return value; + } + } public SecurityManager(SecurityManager parent) throws ConfigurationException { Modified: qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java?rev=1380625&r1=1380624&r2=1380625&view=diff ============================================================================== --- qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java (original) +++ qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/framing/AMQShortString.java Tue Sep 4 13:18:32 2012 @@ -110,7 +110,7 @@ public final class AMQShortString implem { return new LinkedHashMap<AMQShortString, AMQShortString>() { - + @Override protected boolean removeEldestEntry(Map.Entry<AMQShortString, AMQShortString> eldest) { return size() > LOCAL_INTERN_CACHE_SIZE; @@ -845,22 +845,15 @@ public final class AMQShortString implem return internString; } - - public static void main(String args[]) + public static String toString(AMQShortString amqShortString) { - AMQShortString s = new AMQShortString("a.b.c.d.e.f.g.h.i.j.k"); - AMQShortString s2 = s.substring(2, 7); - - AMQShortStringTokenizer t = s2.tokenize((byte) '.'); - while(t.hasMoreTokens()) - { - System.err.println(t.nextToken()); - } + return amqShortString == null ? null : amqShortString.asString(); } - public static String toString(AMQShortString amqShortString) + public static void clearLocalCache() { - return amqShortString == null ? null : amqShortString.asString(); + _localInternMap.remove(); + _localStringMap.remove(); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
