admin@glassfish.java.net

Re: CODE REVIEW: For the additional changes in AMX (along with interceptor changes)

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Mon, 08 Jan 2007 10:14:35 -0800

Kedar,

Thanks for reviewing.

Yes, Logging code (FileandSysLogHandler, etc) has explicit checks for
recursive calls. See the code for the gory details. The recursive
problems won't occur in general, but can occur for any code invoked
directly or indirectly via a Logger as the result of Logging, such as
the AMXLoggingHook. This is what I've fixed (for AMXLoggingHook).
But in general, it's clear from the logging code that Hemanth
experienced similar problems in developing it; locks and flags are
checked internally for this situation.

Yes, a private method is inherently final, but someone could change
it to protected. The JLS language spec could disallow this
combination, but it *doesn't*, so while redundant, it makes the
intention explicit, should someone decide they'd like to change the
method.

An 'Error' in initLogging() was thrown because no logging can be done
at that point, and Exceptions were being ignored. This possibly
could be changed now, but I'd rather leave it alone than change and
retest.

Lloyd


On Jan 6, 2007, at 8:32 AM, kedar wrote:

> Lloyd,
>
> I am reviewing the SunoneInterceptor changes soon, but here is
> this review first ...
>
> See inline.
>
> Another request -- send cvs diff "-u", please.
>
>> AMXLoggingHook.getInstance().enableLoggingHook() is now called
>> from AdminService.start(). This avoids recursive logging problems
>> with trying to fully initialize AMXLogging before PEMain.main()
>> has run. The change also eliminates the somewhat convoluted
>> initialization sequence for AMXLogging.
>>
> Recursive logging problems? That sounds serious. You mean to
> say we have those at the moment?
>>
>> Lloyd
>>
>>
>> RCS file: /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/
>> server/logging/FileandSyslogHandler.java,v
>> retrieving revision 1.6
>> diff -r1.6 FileandSyslogHandler.java
>> 251c251
>> < mAMXLoggingHook = createAMXLoggingHook();
>> ---
>> > mAMXLoggingHook = AMXLoggingHook.getInstance();
>>
> OK.
>>
>> MB2:/gf/build/glassfish lloyd$ cvs diff -u -w appserv-core/src/
>> java/com/sun/enterprise/server/logging/AMXLoggingHook.java
>> Index: appserv-core/src/java/com/sun/enterprise/server/logging/
>> AMXLoggingHook.java
>> ===================================================================
>> RCS file: /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/
>> server/logging/AMXLoggingHook.java,v
>> retrieving revision 1.4
>> diff -u -w -r1.4 AMXLoggingHook.java
>> --- appserv-core/src/java/com/sun/enterprise/server/logging/
>> AMXLoggingHook.java 25 Dec 2005 04:16:26 -0000 1.4
>> +++ appserv-core/src/java/com/sun/enterprise/server/logging/
>> AMXLoggingHook.java 6 Jan 2007 14:49:36 -0000
>> @@ -34,16 +34,19 @@
>> import java.lang.reflect.Method;
>> import java.lang.reflect.Constructor;
>> +import com.sun.enterprise.util.FeatureAvailability;
>> +
>> +import com.sun.appserv.management.helper.AMXDebugHelper;
>> +
>> /**
>> Hook used for AMX logging by FileandSysLogHandler.
>> <b>This class assumes that the caller is holding a lock that
>> will prevent more than one thread from calling {_at_link
>> #publish}.</b>
>> */
>> -final class AMXLoggingHook // don't make this class public
>> +public final class AMXLoggingHook // don't make this class public
> Remove the comment. This class IS public.
>> {
>> private ObjectName mLoggingObjectName;
>> private MBeanServer mMBeanServer;
>> - private boolean mLoggingAvailable;
>> private LoggingImplHook mLoggingImplHook;
>>
>> /**
>> @@ -54,19 +57,11 @@
>> */
>> private Level mMinimumLogLevel;
>>
>> - // /** signature for LOGGING_HOOK_METHOD_NAME method */
>> - // private static final String[] LOGGING_HOOK_SIG = new
>> String[]
>> - // { LogRecord.class.getName(), Formatter.class.getName() };
>> -
>> - // /** name of the operation called on the AMX Logging MBean */
>> - // private static final String LOGGING_HOOK_NAME =
>> "privateLoggingHook";
>> -
>> - private static final boolean DEBUG = false;
>> - private final Output mOut;
>> + private final AMXDebugHelper mDebug;
>> private final String mServerName;
>>
>> private void
>> - dumpSystemProps( final Output output )
>> + dumpSystemProps( final AMXDebugHelper output )
>> {
>> final java.util.Properties props =
>> System.getProperties();
>>
>> @@ -79,93 +74,90 @@
>>
>> }
>>
>> + private static final AMXLoggingHook INSTANCE = new
>> AMXLoggingHook();
>> +
>> + public static AMXLoggingHook
>> + getInstance()
>> + {
>> + return INSTANCE;
>> + }
>> +
>> AMXLoggingHook()
>> {
>> - mServerName = System.getProperty(
>> -
>> com.sun.enterprise.util.SystemPropertyConstants.SERVER_NAME);
>> + mServerName = System.getProperty
>> ( com.sun.enterprise.util.SystemPropertyConstants.SERVER_NAME);
>> final String instanceRoot = System.getProperty(
>>
>> com.sun.enterprise.util.SystemPropertyConstants.INSTANCE_ROOT_PROPERT
>> Y );
>>
>> - mOut = createOutput( DEBUG, instanceRoot + "/
>> AMXLoggingHook-" + mServerName + ".debug" );
>> + mDebug = new AMXDebugHelper( instanceRoot + "/
>> AMXLoggingHook-" + mServerName + ".debug" );
>> +
>> // debug
>> ( "\n________________________________________________________________
>> _______________");
>> // debug( "AMXLoggingHook: REV = 1");
>> // debug( "AMXLoggingHook: started at " + new
>> java.util.Date() + " for server " + mServerName);
>>
>> - // dumpSystemProps( mOut );
>> + // dumpSystemProps( mDebug );
>> mLoggingObjectName = null;
>> - mMBeanServer = getMBeanServer(); // may or may not
>> be available
>> -
>> mMinimumLogLevel = Level.FINEST;
>> mLoggingImplHook = null;
>>
>> - }
>> -
>> - private static Output
>> - createOutput( final boolean debug, final String fileName )
>> - {
>> - try
>> - {
>> - final java.io.File f = new java.io.File( fileName );
>> + //mDebug.println
>> ( com.sun.appserv.management.util.misc.ExceptionUtil.toString( new
>> Exception("HELLO") ) );
>>
>> - return debug ? new FileOutput( f ) : new NullOutput();
>> - }
>> - catch( Throwable t )
>> - {
>> - // nothing can be done; fall through and return
>> NullOutput
>> + // can't initialize now; FileandSysLogHandler is called
>> even before main()
>> + mMBeanServer = null;
>> }
>> + private final void debug( final Object o ) { mDebug.println
>> ( o ); }
> A private method is always final, isn't it or am I misreading the JLS?
>>
>> - return new NullOutput();
>> - }
>> + private static final String LOGGING_IMPL_CLASSNAME =
>> + "com.sun.enterprise.management.ext.logging.LoggingImpl";
>>
>> - private final void debug( final Object o ) { mOut.println
>> ( o.toString() ); }
>> - private MBeanServer
>> - getMBeanServer()
>> - {
>> - if ( mMBeanServer == null )
>> - {
>> - mMBeanServer =
>> -
>> com.sun.enterprise.admin.common.MBeanServerFactory.getMBeanServer();
>> - // may still be null if not yet available
>> - if ( mMBeanServer != null )
>> + /**
>> + Called exactly once to install the Logging MBean and turn
>> on AMX support for logging.
>> + @return ObjectName of AMX Logging MBean
>> + */
>> + public static ObjectName
>> + enableLoggingHook()
>> {
>> - debug( "MBeanServer NOW EXISTS, creating
>> LoggingImpl" );
>> - initLogging( mMBeanServer );
>> - }
>> + return getInstance()._enableLoggingHook();
>> }
>>
>> - return mMBeanServer;
>> + private synchronized ObjectName
>> + _enableLoggingHook()
>> + {
>> + debug( "_enableLoggingHook" );
>> + if ( mLoggingImplHook != null )
>> + {
>> + throw new IllegalStateException();
>> }
>>
>> + mMBeanServer = (MBeanServer)
>> + FeatureAvailability.getInstance().waitForFeature
>> ( FeatureAvailability.MBEAN_SERVER_FEATURE, "AMXLoggingHook");
>>
>> - private static final String LOGGING_IMPL_CLASSNAME =
>> - "com.sun.enterprise.management.ext.logging.LoggingImpl";
>> - /**
>> - Called once to install the Logging MBean
>> - */
>> - private void
>> - initLogging( final MBeanServer mbeanServer )
>> - {
>> + LoggingImplHook hook = null;
>> try
>> {
>> final Class loggingClass = Class.forName
>> ( LOGGING_IMPL_CLASSNAME );
>>
>> final Constructor constructor =
>> loggingClass.getConstructor( String.class );
>> - mLoggingImplHook = (LoggingImplHook)
>> constructor.newInstance( mServerName );
>> + hook = (LoggingImplHook)constructor.newInstance
>> ( mServerName );
>>
>> final Method getObjectNameMethod =
>> loggingClass.getMethod( "getObjectName",
>> String.class );
>> - mLoggingObjectName = (ObjectName)
>> getObjectNameMethod.invoke( mLoggingImplHook, mServerName );
>> - debug( "registering Logging as: " +
>> mLoggingObjectName );
>> - mbeanServer.registerMBean( mLoggingImplHook,
>> mLoggingObjectName );
>> + final ObjectName proposedObjectName = (ObjectName)
>> getObjectNameMethod.invoke( hook, mServerName );
>> + debug( "registering Logging as: " +
>> proposedObjectName );
>> + mLoggingObjectName = mMBeanServer.registerMBean
>> ( hook, proposedObjectName ).getObjectName();
>> + mLoggingImplHook = hook;
>> }
>> catch ( Exception e )
>> {
>> + hook = null;
>> final String msg = "Can't load " +
>> LOGGING_IMPL_CLASSNAME + ", caught: " + e;
>> debug( msg );
>> - throw new Error( msg );
>> + throw new Error( msg, e);
> You are throwing an "Error"?
> Why?
>>
>
> Please correct the above.
> Rest looks all right.
>
> Thanks,
> Kedar
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>