Re: Garbage collection race condition before final checks

[Mandy Chung]

Gary,

Thanks for picking up this bug and fixing this intermittent issue.  
PlatformLoggingMXBeanTest.java in the same directory has the same 
issue.  It'd be good to fix that with the same CR.  These tests were 
copied from test/java/util/logging/LoggingMXBeanTest.java.  I haven't 
looked in details but I wonder why the test/java/util/logging tests 
don't have this intermittent issue and I suspect it holds a strong 
reference.

A couple comment to the fix:

On 11/8/2011 7:35 AM, Gary Adams wrote:

diff --git 
a/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java 
b/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
--- 
a/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java
+++ 
b/test/java/lang/management/PlatformLoggingMXBean/LoggingMXBeanTest.java

Please add the CR number to @bug tag.

@@ -42,6 +42,9 @@
     static String LOGGER_NAME_1 = "com.sun.management.Logger";
     static String LOGGER_NAME_2 = "com.sun.management.Logger.Logger2";
     static String UNKNOWN_LOGGER_NAME = "com.sun.management.Unknown";
+    static Logger logger1;
+    static Logger logger2;

It'd be good to add a comment why you want to keep a strong reference to 
the logger.  Minor nit: could make them as instance variables as they 
are part of the LoggingMXBeanTest instance.

+    static LoggingMXBeanTest testp;

     public static void main(String[] argv) throws Exception {
         MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
@@ -51,7 +54,7 @@
                 LoggingMXBean.class);

         // test LoggingMXBean proxy
-        LoggingMXBeanTest p = new LoggingMXBeanTest(proxy);
+        testp = new LoggingMXBeanTest(proxy);

Could we make checkAttributes as an instance method to LoggingMXBeanTest 
object so that you don't need the static variable?


         // check if the attributes implemented by PlatformLoggingMXBean
         // and LoggingMXBean return the same value
@@ -59,14 +62,18 @@
             ManagementFactory.getPlatformMXBean(mbs, 
PlatformLoggingMXBean.class);

         checkAttributes(proxy, mxbean);
+
+    logger1 = null;
+    logger2 = null;
+    testp = null;

With the above suggested change, I think these 3 lines are not needed.
     }

     // same verification as in java/util/logging/LoggingMXBeanTest2
     public LoggingMXBeanTest(LoggingMXBean mbean) throws Exception {

-        Logger logger1 = Logger.getLogger( LOGGER_NAME_1 );
+        logger1 = Logger.getLogger( LOGGER_NAME_1 );
         logger1.setLevel(Level.FINE);
-        Logger logger2 = Logger.getLogger( LOGGER_NAME_2 );
+        logger2 = Logger.getLogger( LOGGER_NAME_2 );
         logger2.setLevel(null);

         /*
@@ -207,6 +214,18 @@
         // verify logger names
         List<String> loggers1 = mxbean1.getLoggerNames();
         List<String> loggers2 = mxbean2.getLoggerNames();
+
+    // Print the two logger lists for diagnostic purposes
+    System.out.println("  : LoggingMXBean " + loggers1);
+    System.out.println("  : PlatformLoggingMXBean " + loggers2);
+
+    // Print the list of logger level comparisons for diagnostic 
purposes
+        for (String logger : loggers1) {
+        System.out.println ("    : Level (" + logger
+            + ", " + mxbean1.getLoggerLevel(logger)
+            + ", " + mxbean2.getLoggerLevel(logger) + ")");
+    }


It might be good to keep these diagnostic message to be printed only 
when it throws an exception?  Extra diagnostic information is good but 
just want to keep the volume of traces reasonable not to make the output 
harder to use.

Thanks again for fixing it.
Mandy