admin@glassfish.java.net

CODE REVIEW: bugs 1727, 1800, 1801

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Mon, 18 Dec 2006 20:36:07 -0800

Please see:

https://glassfish.dev.java.net/issues/show_bug.cgi?id=1801
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1727
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1800

These diffs cover all 3 issues. I had previously sent diffs for 1727
and 1800, but no response on those so far.

Lloyd
-------------

admin/mbeanapi-impl/src/java/com/sun/enterprise/management/support/
TypeInfos.java admin/mbeanapi-impl/src/java/com/sun/enterprise/
management/support/MBeanInfoConverter.java admin/mbeanapi-impl/src/
java/com/sun/enterprise/management/support/AMXImplBase.java appserv-
core/src/java/com/sun/enterprise/admin/server/core/AdminService.java


MB2:/gf/build/glassfish lloyd$ cvs diff -u
^Ccvs [diff aborted]: received interrupt signal
MB2:/gf/build/glassfish lloyd$ cvs diff -u admin/mbeanapi-impl/src/
java/com/sun/enterprise/management/support/TypeInfos.java admin/
mbeanapi-impl/src/java/com/sun/enterprise/management/support/
MBeanInfoConverter.java admin/mbeanapi-impl/src/java/com/sun/
enterprise/management/support/AMXImplBase.java appserv-core/src/java/
com/sun/enterprise/admin/server/core/AdminService.java
Index: admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/TypeInfos.java
===================================================================
RCS file: /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/
enterprise/management/support/TypeInfos.java,v
retrieving revision 1.16
diff -u -r1.16 TypeInfos.java
--- admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/TypeInfos.java 1 Dec 2006 17:50:26 -0000 1.16
+++ admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/TypeInfos.java 19 Dec 2006 04:32:50 -0000
@@ -40,18 +40,24 @@
import java.util.Iterator;
import javax.management.ObjectName;
+import javax.management.MBeanInfo;
+import javax.management.MBeanAttributeInfo;
import com.sun.appserv.management.util.misc.ListUtil;
import com.sun.appserv.management.util.misc.GSetUtil;
import com.sun.appserv.management.util.misc.ExceptionUtil;
+import com.sun.appserv.management.util.misc.StringUtil;
+import com.sun.appserv.management.helper.AMXDebugHelper;
import com.sun.appserv.management.util.jmx.JMXUtil;
import com.sun.appserv.management.util.stringifier.SmartStringifier;
import com.sun.appserv.management.base.XTypes;
+import com.sun.appserv.management.base.Extra;
import static com.sun.appserv.management.base.XTypes.*;
import com.sun.appserv.management.base.Util;
import com.sun.appserv.management.j2ee.J2EETypes;
+import com.sun.enterprise.util.FeatureAvailability;
/**
         Maps j2eeType to the AMX interface and implementation class
for all AMX
@@ -59,10 +65,14 @@
   */
public final class TypeInfos
{
- private static TypeInfos INSTANCE;
+ private static volatile TypeInfos INSTANCE;

         private final Map<String,TypeInfo> mTypeToInfoMap;
+ private final Map<Class,MBeanInfo> mMBeanInfos;
+
+ private static final String TYPE_INFOS_FEATURE = "TypeInfos";
+
          private static java.util.logging.Logger
      getLogger()
      {
@@ -71,12 +81,73 @@

                 private
         TypeInfos( )
- throws ClassNotFoundException
         {
                 mTypeToInfoMap = new
HashMap<String,TypeInfo>();
-
+
                 initMap( );
+
+ mMBeanInfos = new HashMap<Class, MBeanInfo>();
+ populateMBeanInfos();
+ }
+
+ private void
+ populateMBeanInfos( )
+ {
+ final MBeanAttributeInfo[] extra = getExtraAttributeInfos();
+
+ // create the MBeanInfo from the interfaces
+ for( final TypeInfo typeInfo: mTypeToInfoMap.values() )
+ {
+ final Class theInterface = typeInfo.getInterface();
+
+ final MBeanInfo info =
+ MBeanInfoConverter.getInstance().convert
( theInterface, extra );
+
+ mMBeanInfos.put( theInterface, info );
+ }
+ }
+
+
+ private static final String[] EXTRA_REMOVALS =
+ {
+ "ProxyFactory",
+ "ConnectionSource",
+ "MBeanInfo",
+ "AllAttributes",
+ };
+
+ /**
+ A design decision was to not include certain
Attributes or pseuod-Attributes directly
+ in AMX, so these fields are separated out into
'Extra'. However, some of these
+ are real Attributes that do need to be present in the
MBeanInfo supplied by each
+ MBean.
+ */
+ private static MBeanAttributeInfo[]
+ getExtraAttributeInfos()
+ {
+ final MBeanAttributeInfo[] extraInfos =
+ JMXUtil.interfaceToMBeanInfo( Extra.class ).getAttributes
();
+
+ // remove items that are client-side constructs; not real
Attributes
+ final Map<String,MBeanAttributeInfo> m =
JMXUtil.attributeInfosToMap( extraInfos );
+ for( int i = 0; i < EXTRA_REMOVALS.length; ++i )
+ {
+ m.remove( EXTRA_REMOVALS[ i ] );
+ }
+
+ final MBeanAttributeInfo[] result = new
MBeanAttributeInfo[ m.values().size() ];
+ m.values().toArray( result );
+
+ return( result );
         }
+
+
+ public MBeanInfo
+ getMBeanInfoForInterface( final Class theInterface )
+ {
+ // does not need to be synchronized
+ return mMBeanInfos.get( theInterface );
+ }

      /**
          Optional pre-initialization via thread.
@@ -86,7 +157,8 @@
          public void run()
          {
              // force loading in a separate thread
- TypeInfos.getInstance();
+ INSTANCE = new TypeInfos();
+ FeatureAvailability.getInstance().registerFeature
( TYPE_INFOS_FEATURE, INSTANCE );
          }

          public static void go()
@@ -98,39 +170,35 @@
          }
      };

- public static synchronized void
+ /**
+ Call exactly once, and before getInstance() is called.
+ */
+ public static void
      preload()
      {
- if ( INSTANCE == null )
+ if ( INSTANCE != null )
          {
- Initer.go();
+ throw new IllegalStateException();
          }
+
+ Initer.go();
      }
- public static synchronized TypeInfos
+ public static TypeInfos
         getInstance()
         {
- try
- {
- if ( INSTANCE == null )
- {
- INSTANCE = new TypeInfos( );
- }
- }
- catch( ClassNotFoundException e )
+ // 'INSTANCE' must be volatile!
+ if ( INSTANCE != null )
          {
- e.printStackTrace();
- assert( false );
- throw new RuntimeException( e );
- }
- catch( Throwable t )
- {
- t.printStackTrace();
- assert( false );
- throw new RuntimeException( t );
- }
-
- return( INSTANCE );
+ return INSTANCE;
+ }
+
+ final long start = System.nanoTime();
+ // wait until its ready
+ FeatureAvailability.getInstance().waitForFeature
( TYPE_INFOS_FEATURE, "TypeInfos.getInstance()" );
+ final long elapsed = System.nanoTime() - start;
+
+ return INSTANCE;
         }
                 private void
@@ -148,7 +216,6 @@
                 private void
         initData()
- throws ClassNotFoundException
         {
                 final Object[] data = DATA;
                 for( int i = 0; i < data.length; ++i )
@@ -708,7 +775,6 @@
          */
                 private void
         initMap()
- throws ClassNotFoundException
         {
                 initData();
                 initChildAndContaineeTypes();
Index: admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/MBeanInfoConverter.java
===================================================================
RCS file: /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/
enterprise/management/support/MBeanInfoConverter.java,v
retrieving revision 1.5
diff -u -r1.5 MBeanInfoConverter.java
--- admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/MBeanInfoConverter.java 9 Mar 2006 20:30:47 -0000 1.5
+++ admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/MBeanInfoConverter.java 19 Dec 2006 04:32:50 -0000
@@ -78,12 +78,21 @@
                 return( INSTANCE );
         }
+ private static final Map<String,Class> NAMES_TO_CLASSES =
+ Collections.synchronizedMap( new HashMap<String,Class>() );
+
                 protected static Class
         toClass( final String className )
         {
+ Class theClass = NAMES_TO_CLASSES.get( className );
+ if ( theClass != null )
+ return theClass;
+
                 try
                 {
- return( ClassUtil.getClassFromName
( className ) );
+ theClass = ClassUtil.getClassFromName
( className );
+ NAMES_TO_CLASSES.put( className, theClass );
+ return theClass;
                 }
                 catch( ClassNotFoundException e )
                 {
@@ -270,33 +279,29 @@
         {
                 MBeanInfo result = null;
- // synchronize on theInterface so that we only ever
do it once
- synchronized( theInterface )
- {
- result = find( theInterface );
- if ( result == null )
- {
- final MBeanInfo origInfo =
JMXUtil.interfaceToMBeanInfo( theInterface );
-
- final MBeanAttributeInfo[]
origAttrInfos = origInfo.getAttributes();
- final MBeanAttributeInfo[]
attrInfos = extraAttributeInfos == null ?
- origAttrInfos :
-
JMXUtil.mergeMBeanAttributeInfos( origAttrInfos, extraAttributeInfos);
-
- result = new MBeanInfo(
- origInfo.getClassName(),
-
origInfo.getDescription(),
- convertAttributes
( attrInfos ),
-
origInfo.getConstructors(),
- convertOperations
( origInfo.getOperations() ),
-
origInfo.getNotifications() );
-
- mConvertedInfos.put( theInterface,
result );
- }
- }
-
- return( result );
- }
+ result = find( theInterface );
+ if ( result == null )
+ {
+ // no big deal if this were threaded; we'd do it twice; last
one wins
+ final MBeanInfo origInfo =
JMXUtil.interfaceToMBeanInfo( theInterface );
+
+ final MBeanAttributeInfo[] origAttrInfos =
origInfo.getAttributes();
+ final MBeanAttributeInfo[] attrInfos =
extraAttributeInfos == null ?
+ origAttrInfos :
+ JMXUtil.mergeMBeanAttributeInfos( origAttrInfos,
extraAttributeInfos);
+
+ result = new MBeanInfo(
+ origInfo.getClassName(),
+ origInfo.getDescription(),
+ convertAttributes( attrInfos ),
+ origInfo.getConstructors(),
+ convertOperations( origInfo.getOperations() ),
+ origInfo.getNotifications() );
+
+ mConvertedInfos.put( theInterface, result );
+ }
+ return( result );
+ }
}
Index: admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/AMXImplBase.java
===================================================================
RCS file: /cvs/glassfish/admin/mbeanapi-impl/src/java/com/sun/
enterprise/management/support/AMXImplBase.java,v
retrieving revision 1.21
diff -u -r1.21 AMXImplBase.java
--- admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/AMXImplBase.java 2 Dec 2006 04:02:10 -0000 1.21
+++ admin/mbeanapi-impl/src/java/com/sun/enterprise/management/
support/AMXImplBase.java 19 Dec 2006 04:32:50 -0000
@@ -140,15 +140,16 @@
         /**
                 The interface this MBean implements
         */
- private Class mInterface;
+ private final Class mInterface;
         /**
                 The MBeanInfo
         */
- private MBeanInfo mMBeanInterfaceMBeanInfo;
+ private final MBeanInfo mMBeanInterfaceMBeanInfo;
         /**
                 The parent MBean for this MBean.
+ No need to make volatile; a recompute is OK if one thread
doesn't yet see it.
         */
         private ObjectName mCachedContainerObjectName;
@@ -159,17 +160,16 @@
         private final boolean
mEmitAttributeChangeNotifications;
- private QueryMgr mQueryMgr;
- private AMX mSelfProxy;
+ private volatile QueryMgr mQueryMgr;
+ private volatile AMX mSelfProxy;
         private ConnectionSource mConnectionSource;
         /**
                 An optional Delegate
         */
- private Delegate mSuppliedDelegate;
- private Delegate mDelegate;
-
+ private final Delegate mSuppliedDelegate;
+ private volatile Delegate mDelegate;
         private AMXAttributeNameMapper mAttributeNameMapper = null;
@@ -180,6 +180,11 @@
         protected CoverageInfo mCoverage;
+ /**
+ Maps a j2eeType to a Map<String,Class> which maps an
Attribute name to a Class.
+ */
+ private static final Map<String,Map<String,Class>>
ATTRIBUTE_CLASSES =
+ Collections.synchronizedMap( new
HashMap<String,Map<String,Class>>() );
                 protected Class
         getInterface( final String j2eeType )
@@ -274,6 +279,24 @@
          return wasEnabled;
      }

+ private synchronized MBeanInfo
+ getInterfaceMBeanInfo(
+ final Class theInterface )
+ {
+ MBeanInfo info = TypeInfos.getInstance
().getMBeanInfoForInterface( theInterface );
+ if ( false || getAMXDebug() )
+ {
+ debug( "Interface " + mInterface.getName() +
+ " has MBeanInfo:\n" +
+ MBeanInfoStringifier.DEFAULT.stringify( info ) );
+
+ info = addDebugMBeanInfo( info );
+ }
+
+ return info;
+ }
+
+
         /**
                 Construct a new implementation that implements the
supplied mbeanInterface.
@@ -288,7 +311,7 @@
                 final Delegate delegate )
         {
                 super();
-
+
          mCoverage = new CoverageInfoDummy(); // will change
below...

                 if ( delegate != null )
@@ -327,6 +350,9 @@
                 {
                     ((DelegateBase)mSuppliedDelegate).setDebugOutput
( getDebugOutput() );
                 }
+
+ ATTRIBUTE_CLASSES.put( mJ2EEType,
+ Collections.synchronizedMap( new HashMap<String,Class>
() ) );
                 // initialization of mDelegate is deferred until
later; the supplied delegate
                 // may not be in a position to run yet
@@ -336,27 +362,8 @@
                 mAttributeInfos = null;
                 mFullType = null;
- mMBeanInterfaceMBeanInfo =
- MBeanInfoConverter.getInstance().convert
( mInterface, getExtraAttributeInfos());
-
- if ( getAMXDebug() )
- {
- debug( "Interface " + mInterface.getName() +
- " has MBeanInfo:\n" +
- MBeanInfoStringifier.DEFAULT.stringify
( mMBeanInterfaceMBeanInfo ) );
-
- mMBeanInterfaceMBeanInfo = addDebugMBeanInfo
( mMBeanInterfaceMBeanInfo );
- }
+ mMBeanInterfaceMBeanInfo =
getInterfaceMBeanInfo( mInterface );
         }
-
- private static MBeanAttributeInfo[]
EXTRA_ATTRIBUTE_INFOS = null;
- private static final String[] EXTRA_REMOVALS =
- {
- "ProxyFactory",
- "ConnectionSource",
- "MBeanInfo",
- "AllAttributes",
- };
                 public void
         delegateFailed( final Throwable t )
@@ -371,35 +378,6 @@
          return ClassUtil.stripPackageName( this.getClass().getName
() );
      }

- /**
- An design decision was to not include certain
Attributes or pseuod-Attributes directly
- in AMX, so these fields are separated out into
'Extra'. However, some of these
- are real Attributes that do need to be present in the
MBeanInfo supplied by each
- MBean.
- */
- private static synchronized MBeanAttributeInfo[]
- getExtraAttributeInfos()
- {
- if ( EXTRA_ATTRIBUTE_INFOS == null )
- {
- final MBeanAttributeInfo[]
extraInfos =
- JMXUtil.interfaceToMBeanInfo
( Extra.class ).getAttributes();
- final Map<String,MBeanAttributeInfo>
m = JMXUtil.attributeInfosToMap( extraInfos );
-
- // remove items that are client-side
constructs; not real Attributes
- for( int i = 0; i < EXTRA_REMOVALS.length; ++i )
- {
- m.remove( EXTRA_REMOVALS[ i ] );
- }
-
- EXTRA_ATTRIBUTE_INFOS = new
MBeanAttributeInfo[ m.values().size() ];
- m.values().toArray( EXTRA_ATTRIBUTE_INFOS );
-
- }
-
- return( EXTRA_ATTRIBUTE_INFOS );
- }
-
                 protected MBeanInfo
         getMBeanInfoFromInterface()
         {
@@ -547,7 +525,7 @@
         }
- public synchronized Delegate
+ public Delegate
         getDelegate()
         {
                 return( mDelegate );
@@ -792,24 +770,35 @@
                 return( result );
         }
+ /**
+ Called every time an Attribute is obtained via
delegateGetAttribute(), so
+ make sure it's reasonably fast.
+ */
                 private Class<?>
         getAttributeClass( final String attributeName )
                 throws ClassNotFoundException
         {
- final MBeanAttributeInfo[] infos = getMBeanInfo
().getAttributes();
-
- Class theClass = null;
-
- // do it the dumb way; later we can optimize
- for( int i = 0; i < infos.length; ++i )
- {
- if ( infos[ i ].getName().equals
( attributeName ) )
- {
- theClass =
ClassUtil.getClassFromName( infos[ i ].getType() );
- break;
- }
+ final Map<String,Class> mappings = ATTRIBUTE_CLASSES.get
( getJ2EEType() );
+
+ Class theClass = mappings.get( attributeName );
+ // initialize only if the class is null and there isn't a
mapping for to null
+ if ( theClass == null && ! mappings.containsKey
( attributeName ) )
+ {
+ // no need to synchronize; the Map is already so.
+ // And if mappings were somehow 'put' twice, that's rare
and of no importance
+ final MBeanAttributeInfo[] infos = getMBeanInfo
().getAttributes();
+
+ // Map each Attribute to a Class
+ for( int i = 0; i < infos.length; ++i )
+ {
+ final String attrName = infos[ i ].getName();
+ final Class c = ClassUtil.getClassFromName( infos
[ i ].getType() );
+ mappings.put( attrName, c );
+ }
+
+ theClass = mappings.get( attributeName );
                 }
-
+
                 return( theClass );
         }
@@ -856,6 +845,9 @@
                                 getMBeanLogger().warning
( "AMXImplBase.delegateGetAttribute: " +
                                         "Can't find class for
attribute: " + name + "=" + value +
                                         " in object " + getObjectName
() );
+
+ // add a null mapping
+ ATTRIBUTE_CLASSES.get( getJ2EEType() ).put( name,
null );
                         }
                 }
@@ -928,10 +920,7 @@
         {
                 if ( mAttributeInfos == null || !
getMBeanInfoIsInvariant() )
                 {
- synchronized( this )
- {
- mAttributeInfos =
JMXUtil.attributeInfosToMap( getMBeanInfo().getAttributes() );
- }
+ mAttributeInfos = JMXUtil.attributeInfosToMap
( getMBeanInfo().getAttributes() );
                 }
                 return( mAttributeInfos );
@@ -2150,7 +2139,7 @@
                 <li></li>
                 </ul>
         */
- protected ObjectName
+ protected ObjectName
         preRegisterModifyName(
                 final MBeanServer server,
                 final ObjectName nameIn )
@@ -2183,7 +2172,12 @@
                 return( nameOut );
         }
- public final ObjectName
+ /*
+ Note that this method is 'synchronized'--to force visibility
of all fields it affects.
+ Since it's called only once (per instance) for an MBean
Registration, it has no performance
+ impact on later use, but guarantees visibility of all non-
final instance variables.
+ */
+ public final synchronized ObjectName
         preRegister(
                 final MBeanServer server,
                 final ObjectName nameIn)
@@ -2496,7 +2490,13 @@
             }
         }
- public final void
+ /*
+ Note that this method is 'synchronized'--to force visibility
of all fields it affects.
+ Since it's called only once (per instance) for an MBean
Registration, it has no performance
+ impact on later use, but guarantees visibility of all non-
final instance variables, both
+ on this class and all subclasses, since they can only modify
things via postRegisterHook().
+ */
+ public final synchronized void
         postRegister( Boolean registrationSucceeded )
         {
                 super.postRegister( registrationSucceeded );
@@ -2540,7 +2540,7 @@
         /**
                 The QueryMgr is a special-case; all the other types
rely on it.
         */
- public synchronized ObjectName
+ public ObjectName
         getQueryMgrObjectName()
         {
                 ObjectName objectName = null;
@@ -2609,18 +2609,17 @@
                 protected final QueryMgr
         getQueryMgr()
         {
- if ( mQueryMgr == null )
- {
- final ObjectName objectName =
getQueryMgrObjectName();
- if ( objectName != null )
- {
- synchronized( this )
- {
- mQueryMgr = getProxyFactory().getProxy
( objectName, QueryMgr.class);
- }
- }
- }
-
+ // this relies on mQueryMgr being 'volatile'
+ if ( mQueryMgr != null )
+ return mQueryMgr;
+
+ final ObjectName objectName =
getQueryMgrObjectName();
+ if ( objectName != null )
+ {
+ // it doesn't matter if two thread do this; the same
proxy will be returned.
+ mQueryMgr = getProxyFactory().getProxy( objectName,
QueryMgr.class);
+ }
+
                 return( mQueryMgr );
         }
Index: appserv-core/src/java/com/sun/enterprise/admin/server/core/
AdminService.java
===================================================================
RCS file: /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/
admin/server/core/AdminService.java,v
retrieving revision 1.16
diff -u -r1.16 AdminService.java
--- appserv-core/src/java/com/sun/enterprise/admin/server/core/
AdminService.java 19 Dec 2006 01:52:27 -0000 1.16
+++ appserv-core/src/java/com/sun/enterprise/admin/server/core/
AdminService.java 19 Dec 2006 04:32:50 -0000
@@ -179,8 +179,21 @@
       */
      private AdminService() {
          sMBeanServerID = kDefaultImpl; //the only implementation
for now
+
+ preloadAMXTypeInfos();
      }
+ private static void preloadAMXTypeInfos() {
+ try {
+ final Class c = Class.forName
( "com.sun.enterprise.management.support.TypeInfos" );
+ final java.lang.reflect.Method m = c.getMethod
( "preload", (Class[])null );
+ m.invoke( (Object[])null );
+ }
+ catch( Exception e ) {
+ throw new Error( e );
+ }
+ }
+
      /**
       * Create admin service. Admin Service is initialized by an
internal
       * lifecycle module (AdminServiceLifeCycle).
MB2:/gf/build/glassfish lloyd$