From c68be0fed37284719405af69ded6bc14ca723694 Mon Sep 17 00:00:00 2001 From: Robert Burrell Donkin Date: Sun, 12 Feb 2006 20:01:28 +0000 Subject: [PATCH] Added guards around diagnostic logging. Is unlikely to have much effect on real life performance but is good practice and should stop questions. git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/logging/trunk@377229 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/commons/logging/LogFactory.java | 216 +++++++++++------- .../commons/logging/impl/LogFactoryImpl.java | 150 ++++++++---- 2 files changed, 232 insertions(+), 134 deletions(-) diff --git a/src/java/org/apache/commons/logging/LogFactory.java b/src/java/org/apache/commons/logging/LogFactory.java index e59d537..060d965 100644 --- a/src/java/org/apache/commons/logging/LogFactory.java +++ b/src/java/org/apache/commons/logging/LogFactory.java @@ -386,7 +386,9 @@ public abstract class LogFactory { // This is an odd enough situation to report about. This // output will be a nuisance on JDK1.1, as the system // classloader is null in that environment. - logDiagnostic("Context classloader is null."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Context classloader is null."); + } } // Return any previously registered factory for this class loader @@ -395,10 +397,12 @@ public abstract class LogFactory { return factory; } - logDiagnostic( - "[LOOKUP] LogFactory implementation requested for the first time for context classloader " - + objectId(contextClassLoader)); - logHierarchy("[LOOKUP] ", contextClassLoader); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] LogFactory implementation requested for the first time for context classloader " + + objectId(contextClassLoader)); + logHierarchy("[LOOKUP] ", contextClassLoader); + } // Load properties file. // @@ -435,25 +439,31 @@ public abstract class LogFactory { // Determine which concrete LogFactory subclass to use. // First, try a global system property - logDiagnostic( - "[LOOKUP] Looking for system property [" + FACTORY_PROPERTY - + "] to define the LogFactory subclass to use..."); - + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] Looking for system property [" + FACTORY_PROPERTY + + "] to define the LogFactory subclass to use..."); + } + try { String factoryClass = System.getProperty(FACTORY_PROPERTY); if (factoryClass != null) { - logDiagnostic( - "[LOOKUP] Creating an instance of LogFactory class '" + factoryClass - + "' as specified by system property " + FACTORY_PROPERTY); - + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] Creating an instance of LogFactory class '" + factoryClass + + "' as specified by system property " + FACTORY_PROPERTY); + } + factory = newFactory(factoryClass, baseClassLoader, contextClassLoader); } } catch (SecurityException e) { - logDiagnostic( - "[LOOKUP] A security exception occurred while trying to create an" - + " instance of the custom factory class" - + ": [" + e.getMessage().trim() - + "]. Trying alternative implementations..."); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] A security exception occurred while trying to create an" + + " instance of the custom factory class" + + ": [" + e.getMessage().trim() + + "]. Trying alternative implementations..."); + } ; // ignore } @@ -465,10 +475,11 @@ public abstract class LogFactory { // that implements the desired interface. if (factory == null) { - logDiagnostic( - "[LOOKUP] Looking for a resource file of name [" + SERVICE_ID - + "] to define the LogFactory subclass to use..."); - + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] Looking for a resource file of name [" + SERVICE_ID + + "] to define the LogFactory subclass to use..."); + } try { InputStream is = getResourceAsStream(contextClassLoader, SERVICE_ID); @@ -488,23 +499,24 @@ public abstract class LogFactory { if (factoryClassName != null && ! "".equals(factoryClassName)) { - - logDiagnostic( - "[LOOKUP] Creating an instance of LogFactory class " + factoryClassName - + " as specified by file '" + SERVICE_ID - + "' which was present in the path of the context" - + " classloader."); - + if (isDiagnosticsEnabled()) { + logDiagnostic( + "[LOOKUP] Creating an instance of LogFactory class " + factoryClassName + + " as specified by file '" + SERVICE_ID + + "' which was present in the path of the context" + + " classloader."); + } factory = newFactory(factoryClassName, baseClassLoader, contextClassLoader ); } } } catch( Exception ex ) { - logDiagnostic( + if (isDiagnosticsEnabled()) { + logDiagnostic( "[LOOKUP] A security exception occurred while trying to create an" + " instance of the custom factory class" + ": [" + ex.getMessage().trim() + "]. Trying alternative implementations..."); - + } ; // ignore } } @@ -519,16 +531,18 @@ public abstract class LogFactory { // system property ) if (factory == null) { - logDiagnostic( - "[LOOKUP] Looking for a properties file of name '" + FACTORY_PROPERTIES - + "' to define the LogFactory subclass to use..."); - - if (props != null) { + if (isDiagnosticsEnabled()) { logDiagnostic( + "[LOOKUP] Looking for a properties file of name '" + FACTORY_PROPERTIES + + "' to define the LogFactory subclass to use..."); + } + if (props != null) { + if (isDiagnosticsEnabled()) { + logDiagnostic( "[LOOKUP] Properties file found. Looking for property '" + FACTORY_PROPERTY + "' to define the LogFactory subclass to use..."); - + } String factoryClass = props.getProperty(FACTORY_PROPERTY); if (factoryClass != null) { factory = newFactory(factoryClass, baseClassLoader, contextClassLoader); @@ -542,10 +556,12 @@ public abstract class LogFactory { // Fourth, try the fallback implementation class if (factory == null) { - logDiagnostic( + if (isDiagnosticsEnabled()) { + logDiagnostic( "[LOOKUP] Loading the default LogFactory implementation '" + FACTORY_DEFAULT + "' via the same classloader that loaded this LogFactory" + " class (ie not looking in the context classloader)."); + } // Note: unlike the above code which can try to load custom LogFactory // implementations via the TCCL, we don't try to load the default LogFactory @@ -625,7 +641,9 @@ public abstract class LogFactory { */ public static void release(ClassLoader classLoader) { - logDiagnostic("Releasing factory for classloader " + objectId(classLoader)); + if (isDiagnosticsEnabled()) { + logDiagnostic("Releasing factory for classloader " + objectId(classLoader)); + } synchronized (factories) { if (classLoader == null) { if (nullClassLoaderFactory != null) { @@ -654,7 +672,9 @@ public abstract class LogFactory { */ public static void releaseAll() { - logDiagnostic("Releasing factory for all classloaders."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Releasing factory for all classloaders."); + } synchronized (factories) { Enumeration elements = factories.elements(); while (elements.hasMoreElements()) { @@ -700,9 +720,11 @@ public abstract class LogFactory { try { return clazz.getClassLoader(); } catch(SecurityException ex) { - logDiagnostic( - "Unable to get classloader for class " + clazz - + " due to security restrictions."); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Unable to get classloader for class " + clazz + + " due to security restrictions."); + } throw ex; } } @@ -937,16 +959,18 @@ public abstract class LogFactory { if (result instanceof LogConfigurationException) { LogConfigurationException ex = (LogConfigurationException) result; - logDiagnostic( - "An error occurred while loading the factory class:" - + ex.getMessage()); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "An error occurred while loading the factory class:" + + ex.getMessage()); + } throw ex; } - - logDiagnostic( - "Created object " + objectId(result) - + " to manage classloader " + objectId(contextClassLoader)); - + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Created object " + objectId(result) + + " to manage classloader " + objectId(contextClassLoader)); + } return (LogFactory)result; } @@ -993,9 +1017,11 @@ public abstract class LogFactory { // to be generated/caught & recast properly. logFactoryClass = classLoader.loadClass(factoryClass); if (LogFactory.class.isAssignableFrom(logFactoryClass)) { - logDiagnostic( - "Loaded class " + logFactoryClass.getName() - + " from classloader " + objectId(classLoader)); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Loaded class " + logFactoryClass.getName() + + " from classloader " + objectId(classLoader)); + } } else { // // This indicates a problem with the ClassLoader tree. @@ -1007,13 +1033,15 @@ public abstract class LogFactory { // The most likely fix for this // problem is to remove the extra JCL jars from the // ClassLoader hierarchy. - // - logDiagnostic( - "Factory class " + logFactoryClass.getName() - + " loaded from classloader " + objectId(classLoader) - + " does not extend '" + LogFactory.class.getName() - + "' as loaded by this classloader."); - logHierarchy("[BAD CL TREE] ", classLoader); + // + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Factory class " + logFactoryClass.getName() + + " loaded from classloader " + objectId(classLoader) + + " does not extend '" + LogFactory.class.getName() + + "' as loaded by this classloader."); + logHierarchy("[BAD CL TREE] ", classLoader); + } } return (LogFactory) logFactoryClass.newInstance(); @@ -1021,20 +1049,24 @@ public abstract class LogFactory { } catch (ClassNotFoundException ex) { if (classLoader == thisClassLoader) { // Nothing more to try, onwards. - logDiagnostic( - "Unable to locate any class called '" + factoryClass - + "' via classloader " + objectId(classLoader)); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Unable to locate any class called '" + factoryClass + + "' via classloader " + objectId(classLoader)); + } throw ex; } // ignore exception, continue } catch (NoClassDefFoundError e) { if (classLoader == thisClassLoader) { // Nothing more to try, onwards. - logDiagnostic( - "Class '" + factoryClass + "' cannot be loaded" - + " via classloader " + objectId(classLoader) - + " - it depends on some other class that cannot" - + " be found."); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Class '" + factoryClass + "' cannot be loaded" + + " via classloader " + objectId(classLoader) + + " - it depends on some other class that cannot" + + " be found."); + } throw e; } // ignore exception, continue @@ -1043,9 +1075,11 @@ public abstract class LogFactory { // This cast exception is not due to classloader issues; // the specified class *really* doesn't extend the // required LogFactory base class. - logDiagnostic( - "Class '" + factoryClass + "' really does not extend '" - + LogFactory.class.getName() + "'"); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Class '" + factoryClass + "' really does not extend '" + + LogFactory.class.getName() + "'"); + } throw e; } // Ignore exception, continue @@ -1068,16 +1102,19 @@ public abstract class LogFactory { */ // Warning: must typecast here & allow exception // to be generated/caught & recast properly. - - logDiagnostic( - "Unable to load factory class via classloader " - + objectId(classLoader) - + " - trying the classloader associated with this LogFactory."); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Unable to load factory class via classloader " + + objectId(classLoader) + + " - trying the classloader associated with this LogFactory."); + } logFactoryClass = Class.forName(factoryClass); return (LogFactory) logFactoryClass.newInstance(); } catch (Exception e) { // Check to see if we've got a bad configuration - logDiagnostic("Unable to create LogFactory instance."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Unable to create LogFactory instance."); + } if (logFactoryClass != null && !LogFactory.class.isAssignableFrom(logFactoryClass)) { @@ -1137,9 +1174,11 @@ public abstract class LogFactory { return ClassLoader.getSystemResources(name); } } catch(IOException e) { - logDiagnostic( - "Exception while trying to find configuration file " - + name + ":" + e.getMessage()); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Exception while trying to find configuration file " + + name + ":" + e.getMessage()); + } return null; } catch(NoSuchMethodError e) { // we must be running on a 1.1 JVM which doesn't support @@ -1174,7 +1213,9 @@ public abstract class LogFactory { return props; } } catch(IOException e) { - logDiagnostic("Unable to read URL " + url); + if (isDiagnosticsEnabled()) { + logDiagnostic("Unable to read URL " + url); + } } return null; @@ -1234,7 +1275,9 @@ public abstract class LogFactory { } } } catch (SecurityException e) { - logDiagnostic("SecurityException thrown"); + if (isDiagnosticsEnabled()) { + logDiagnostic("SecurityException thrown"); + } } return props; } @@ -1400,6 +1443,9 @@ public abstract class LogFactory { * @param classLoader */ private static void logHierarchy(String prefix, ClassLoader classLoader) { + if (!isDiagnosticsEnabled()) { + return; + } ClassLoader systemClassLoader; if (classLoader != null) { final String classLoaderString = classLoader.toString(); @@ -1483,6 +1529,8 @@ public abstract class LogFactory { initDiagnostics(); logClassLoaderEnvironment(LogFactory.class); factories = createFactoryStore(); - logDiagnostic("BOOTSTRAP COMPLETED"); + if (isDiagnosticsEnabled()) { + logDiagnostic("BOOTSTRAP COMPLETED"); + } } } diff --git a/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java b/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java index 9a990b7..f5c434b 100644 --- a/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java +++ b/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java @@ -87,7 +87,9 @@ public class LogFactoryImpl extends LogFactory { public LogFactoryImpl() { super(); initDiagnostics(); // method on this object - logDiagnostic("Instance created."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Instance created."); + } } @@ -637,7 +639,9 @@ public class LogFactoryImpl extends LogFactory { * affect the future behaviour of this class. */ private boolean isLogLibraryAvailable(String name, String classname) { - logDiagnostic("Checking for " + name + "."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Checking for " + name + "."); + } try { Log log = createLogFromClass( classname, @@ -645,14 +649,20 @@ public class LogFactoryImpl extends LogFactory { false); if (log == null) { - logDiagnostic("Did not find " + name + "."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Did not find " + name + "."); + } return false; } else { - logDiagnostic("Found " + name + "."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Found " + name + "."); + } return true; } } catch(LogConfigurationException e) { - logDiagnostic("Logging system " + name + " is available but not useable."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Logging system " + name + " is available but not useable."); + } return false; } } @@ -669,22 +679,33 @@ public class LogFactoryImpl extends LogFactory { * @return the value associated with the property, or null. */ private String getConfigurationValue(String property) { - logDiagnostic("[ENV] Trying to get configuration for item " + property); - - logDiagnostic("[ENV] Looking for attribute " + property); + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Trying to get configuration for item " + property); + } + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Looking for attribute " + property); + } Object valueObj = getAttribute(property); if (valueObj != null) { - logDiagnostic("[ENV] Found value [" + valueObj + "] for " + property); + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Found value [" + valueObj + "] for " + property); + } return valueObj.toString(); } - logDiagnostic("[ENV] Looking for system property " + property); + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Looking for system property " + property); + } try { String value = System.getProperty(property); - logDiagnostic("[ENV] Found value [" + value + "] for " + property); + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Found value [" + value + "] for " + property); + } return value; } catch (SecurityException e) { - logDiagnostic("[ENV] Security prevented reading system property."); + if (isDiagnosticsEnabled()) { + logDiagnostic("[ENV] Security prevented reading system property."); + } } return null; @@ -727,7 +748,9 @@ public class LogFactoryImpl extends LogFactory { private Log discoverLogImplementation(String logCategory) throws LogConfigurationException { - logDiagnostic("Attempting to discover a Log implementation."); + if (isDiagnosticsEnabled()) { + logDiagnostic("Attempting to discover a Log implementation."); + } initConfiguration(); @@ -831,34 +854,46 @@ public class LogFactoryImpl extends LogFactory { */ private String findUserSpecifiedLogClassName() { - logDiagnostic("Trying to get log class from attribute '" + LOG_PROPERTY + "'"); + if (isDiagnosticsEnabled()) { + logDiagnostic("Trying to get log class from attribute '" + LOG_PROPERTY + "'"); + } String specifiedClass = (String) getAttribute(LOG_PROPERTY); if (specifiedClass == null) { // @deprecated - logDiagnostic("Trying to get log class from attribute '" + - LOG_PROPERTY_OLD + "'"); + if (isDiagnosticsEnabled()) { + logDiagnostic("Trying to get log class from attribute '" + + LOG_PROPERTY_OLD + "'"); + } specifiedClass = (String) getAttribute(LOG_PROPERTY_OLD); } if (specifiedClass == null) { - logDiagnostic("Trying to get log class from system property '" + + if (isDiagnosticsEnabled()) { + logDiagnostic("Trying to get log class from system property '" + LOG_PROPERTY + "'"); + } try { specifiedClass = System.getProperty(LOG_PROPERTY); } catch (SecurityException e) { - logDiagnostic("No access allowed to system property '" + + if (isDiagnosticsEnabled()) { + logDiagnostic("No access allowed to system property '" + LOG_PROPERTY + "' - " + e.getMessage()); + } } } if (specifiedClass == null) { // @deprecated - logDiagnostic("Trying to get log class from system property '" + + if (isDiagnosticsEnabled()) { + logDiagnostic("Trying to get log class from system property '" + LOG_PROPERTY_OLD + "'"); + } try { specifiedClass = System.getProperty(LOG_PROPERTY_OLD); } catch (SecurityException e) { - logDiagnostic("No access allowed to system property '" + + if (isDiagnosticsEnabled()) { + logDiagnostic("No access allowed to system property '" + LOG_PROPERTY_OLD + "' - " + e.getMessage()); + } } } @@ -891,7 +926,9 @@ public class LogFactoryImpl extends LogFactory { boolean affectState) throws LogConfigurationException { - logDiagnostic("Attempting to instantiate '" + logAdapterClassName + "'"); + if (isDiagnosticsEnabled()) { + logDiagnostic("Attempting to instantiate '" + logAdapterClassName + "'"); + } Object[] params = { logCategory }; Log logAdapter = null; @@ -1148,11 +1185,13 @@ public class LogFactoryImpl extends LogFactory { // In some classloading setups (e.g. JBoss with its // UnifiedLoaderRepository) this can still work, so if user hasn't // forbidden it, just return the contextClassLoader. - if (allowFlawedContext) { - logDiagnostic( - "Warning: the context classloader is not part of a" - + " parent-child relationship with the classloader that" - + " loaded LogFactoryImpl."); + if (allowFlawedContext) { + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Warning: the context classloader is not part of a" + + " parent-child relationship with the classloader that" + + " loaded LogFactoryImpl."); + } // If contextClassLoader were null, getLowestClassLoader() would // have returned thisClassLoader. The fact we are here means // contextClassLoader is not null, so we can just return it. @@ -1173,12 +1212,14 @@ public class LogFactoryImpl extends LogFactory { // custom classloaders but fail to set the context classloader so // we handle those flawed systems anyway. if (allowFlawedContext) { - logDiagnostic( - "Warning: the context classloader is an ancestor of the" - + " classloader that loaded LogFactoryImpl; it should be" - + " the same or a descendant. The application using" - + " commons-logging should ensure the context classloader" - + " is used correctly."); + if (isDiagnosticsEnabled()) { + logDiagnostic( + "Warning: the context classloader is an ancestor of the" + + " classloader that loaded LogFactoryImpl; it should be" + + " the same or a descendant. The application using" + + " commons-logging should ensure the context classloader" + + " is used correctly."); + } } else { throw new LogConfigurationException( "Bad classloader hierarchy; LogFactoryImpl was loaded via" @@ -1250,10 +1291,12 @@ public class LogFactoryImpl extends LogFactory { ClassLoader classLoader, Throwable discoveryFlaw) { - logDiagnostic("Could not instantiate Log '" + if (isDiagnosticsEnabled()) { + logDiagnostic("Could not instantiate Log '" + logAdapterClassName + "' -- " - + discoveryFlaw.getLocalizedMessage()); - + + discoveryFlaw.getLocalizedMessage()); + } + if (!allowFlawedDiscovery) { throw new LogConfigurationException(discoveryFlaw); } @@ -1326,17 +1369,20 @@ public class LogFactoryImpl extends LogFactory { msg.append("You have more than one version of "); msg.append(Log.class.getName()); msg.append(" visible."); - logDiagnostic(msg.toString()); - + if (isDiagnosticsEnabled()) { + logDiagnostic(msg.toString()); + } throw new LogConfigurationException(msg.toString()); } - StringBuffer msg = new StringBuffer(); - msg.append("Warning: bad log hierarchy. "); - msg.append("You have more than one version of "); - msg.append(Log.class.getName()); - msg.append(" visible."); - logDiagnostic(msg.toString()); + if (isDiagnosticsEnabled()) { + StringBuffer msg = new StringBuffer(); + msg.append("Warning: bad log hierarchy. "); + msg.append("You have more than one version of "); + msg.append(Log.class.getName()); + msg.append(" visible."); + logDiagnostic(msg.toString()); + } } else { // this is just a bad adapter class if (!allowFlawedDiscovery) { @@ -1345,16 +1391,20 @@ public class LogFactoryImpl extends LogFactory { msg.append("Log class "); msg.append(badClass.getName()); msg.append(" does not implement the Log interface."); - logDiagnostic(msg.toString()); - + if (isDiagnosticsEnabled()) { + logDiagnostic(msg.toString()); + } + throw new LogConfigurationException(msg.toString()); } - StringBuffer msg = new StringBuffer(); - msg.append("Warning: Log class "); - msg.append(badClass.getName()); - msg.append(" does not implement the Log interface."); - logDiagnostic(msg.toString()); + if (isDiagnosticsEnabled()) { + StringBuffer msg = new StringBuffer(); + msg.append("Warning: Log class "); + msg.append(badClass.getName()); + msg.append(" does not implement the Log interface."); + logDiagnostic(msg.toString()); + } } } }