admin@glassfish.java.net

CODE REVIEW: FindBugs and more: GeneratedMonitoringMBeanImpl

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Tue, 27 Mar 2007 15:35:19 -0700

Please review no later than noon on Thursday 3/30.

Fixed an "impossible cast" warning as well as a thread safety issue
with 'mbeanInfo'.

Lloyd


RCS file: /cvs/glassfish/admin/monitor/src/java/com/sun/enterprise/
admin/monitor/registry/spi/GeneratedMonitoringMBeanImpl.java,v
retrieving revision 1.3
diff -w -u -r1.3 GeneratedMonitoringMBeanImpl.java
--- admin/monitor/src/java/com/sun/enterprise/admin/monitor/registry/
spi/GeneratedMonitoringMBeanImpl.java 25 Dec 2005 03:43:33
-0000 1.3
+++ admin/monitor/src/java/com/sun/enterprise/admin/monitor/registry/
spi/GeneratedMonitoringMBeanImpl.java 27 Mar 2007 17:46:54 -0000
@@ -52,11 +52,11 @@
   * @author <a href=mailto:shreedhar.ganapathy_at_sun.com>Shreedhar
Ganapathy</a>
   */
public class GeneratedMonitoringMBeanImpl implements DynamicMBean {
- MBeanInfo mbeanInfo;
- Stats resourceInstance;
+ volatile MBeanInfo mbeanInfo;
+ final Stats resourceInstance;
      public static final String LOGGER_NAME="this.is.console";
      final Logger logger;
- Hashtable attributeMap;
+ final Hashtable<String,String> attributeMap;
      //StatsHolder statsHolder = null;
      /**
       * constructs a monitoring mbean that manages an underlying
Stats resource
@@ -64,7 +64,7 @@
      public GeneratedMonitoringMBeanImpl(Stats stats) {
          logger = Logger.getLogger(LOGGER_NAME);
          this.resourceInstance=stats;
- attributeMap=new Hashtable();
+ attributeMap=new Hashtable<String,String>();
      }

      /**
@@ -74,10 +74,22 @@
       * @param javax.management.j2ee.statistics.Stats
       * @param javax.management.MBeanInfo
       */
- MBeanInfo introspect(){
+ final MBeanInfo introspect(){
+ // !!! 'mbeanInfo' must be 'volatile' !!!
+ // this is thread-safe, not the double-null-check idiom so
long as
+ // 'mbeanInfo' is 'volatile'
+ if ( mbeanInfo != null ) {
+ return mbeanInfo;
+ }
+
+ synchronized( this ) {
+ if ( mbeanInfo == null ) {
          ManagedResourceIntrospector mri = new
ManagedResourceIntrospector(this);
          mbeanInfo = mri.introspect(this.resourceInstance);
          setUpAttributeMap();
+ }
+ }
+
          return mbeanInfo;
      }
      /**
@@ -115,9 +127,9 @@
          if(str == null){
              throw new NullPointerException("An attribute needs to
be specified to get value");
          }
- if(mbeanInfo == null){
+
              introspect();
- }
+
          if(!isValidAttribute(str)){
              throw new AttributeNotFoundException("The requested
attribute is not recognized");
          }
@@ -167,8 +179,8 @@
       * @return javax.management.AttributeList
       */
      public javax.management.AttributeList getAttributes(String[]
str) {
- if(mbeanInfo == null)
              introspect();
+
          AttributeList list = new AttributeList();
          try{
              for(int i=0; i<str.length;i++){
@@ -188,15 +200,12 @@
       * @return javax.management.MBeanInfo
       */
      public javax.management.MBeanInfo getMBeanInfo() {
- if(mbeanInfo== null){
              introspect();
- }
          return mbeanInfo;
      }

      public Object invoke(String str, Object[] obj, String[] str2)
throws
      javax.management.MBeanException,
javax.management.ReflectionException{
- if(mbeanInfo == null)
              introspect();
          Object a =null;
          Class[] c = new Class[]{};