Garbage collection race condition before final checks

[Gary Adams]

On 11/8/11 11:13 PM, Mandy Chung wrote:
> 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.

I'll take a look at the other tests to see if they suffer from the
same issue. I was able to force the intermittent bugs to reproduce quickly
with a strategic GC in the worst possible place.
>
> 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.

Will do.
>
>> @@ -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.
I'll give it a try.
>
>> +    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?
My preference is to leave a test as intact as possible
from how the original test was written. e.g. do the minimum
required to make the test reliable. These regression tests
are meant to confirm that a particular issue does not slip
back into the code base. Changing checkAttributes to be
an instance method is not essential to getting the test to
work properly. Better to get 10 intermittent tests to work reliably
rather than get 1 test polished to perfection. $.02
>
>>
>>          // 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.
OK
>>      }
>>
>>      // 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.
We can probably delete the diagnostic output, now that the problem
is clearly identified.
>
> Thanks again for fixing it.
Alan pointed out a pile of "teststabilization" bugs that have been tagged.
I'll see if any of those were suffering from the same issue found here.
> Mandy