jmx-dev RFR: 8020875 java/lang/management/ThreadMXBean/ResetPeakThreadCount.java fails intermittently

[David Holmes]

On 23/07/2013 6:25 PM, Daniel Fuchs wrote:
> Hi Jaroslav,
>
> This looks like a tough problem as it is altogether possible that
> some of the VM daemon threads will terminate during the duration
> of the call - and if that's the case, the condition:
>     new peak >= old peak + delta
> might not even be true.
> I am not a VM specialist so I don't know whether there can be
> such daemon threads that will be arbitrarily started and stopped
> by the VM - but if that happens I don't see how you could work around
> it.
>
> There seems to be something strange in the test though: line 209,
> you catch InterruptedException just to call
> Thread.currentThread().interrupt() and interrupt the thread again??
> Did you mean maybe to call Thread.currentThread().interrupted() instead?

No but good catch as the way this is done is not quite right. The 
re-posting of the interrupt() needs to happen outside the loop otherwise 
the sleep() will simply rethrow the InterruptedException. The normal 
pattern is:

boolean interrupted = false;
while (...) {
   try {
       Thread.sleep(5);
       ...
   }
    catch (InterruptedException ie) {
      interrupted = true;
    }
}
if (interrupted)
    Thread.currentThread().interrupt(); // re-assert interrupt state


Of course it is debatable whether there is any point continuing the loop 
if you do get interrupted (which should never happen anyway).

> There are other places that seems to be prone to failures in this test
> too for instance:
>
> startThreads(...) {
>
>    while(mbean.getThreadCount() < (current + count)) {
>        ...
>    }
>
> }
>
> If the VM can start and stop arbitrary threads then this condition
> seems dubious. There's the same kind of logic in terminateThreads.
> Not sure you can/should do anything about it though - it's
> just to point out that these steps might need to be revisited
> if the test still fails sporadically...
>
> Also I'm not sure that using volatile for the 'live' array will
> work - the array itself is volatile - but does it extends to its
> elements?

No it doesn't.

David
-----

> It might be better to declare the live array as static final and
> use a synchronization block on the array itself when accessing it:
>
> private static final boolean live[] = new boolean[ALL_THREADS];
> private static boolean isAlive(int i) {
>      synchronized(live) { return live[i] };
> }
>
> ...
>
>       synchronized(live) {
>            live[i] == false;
>       }
>
> ...
>
>       while (isAlive[id]) {
>             ...
>       }
>
> ...
>
> best regards,
>
> -- daniel
>
> On 7/22/13 1:55 PM, Jaroslav Bachorik wrote:
>> The java/lang/management/ThreadMXBean/ResetPeakThreadCount.java test
>> seems to be failing intermittently.
>>
>> The test checks the functionality of the
>> j.l.m.ThreadMXBean.resetPeakThreadCount() method. It does so by
>> capturing the current value of "getPeakThreadCount()", starting a
>> predefined number of the user threads, stopping them and resetting the
>> stored peak value and making sure the new peak equals to the number of
>> the actually running threads.
>>
>> The main problem is that it is not possible to prevent JVM to start/stop
>> arbitrary system threads while executing the test. This might lead to
>> small variations of the reported peak (a short-lived system thread is
>> started while the batch of the user threads is running) or the expected
>> number of running threads (again, a short-lived system thread is started
>> at the moment the test asks for the number of running threads).
>>
>> The patch does not fix those shortcomings as it is not really possible
>> to do given the nature of the JVM threading system. It rather tries to
>> relax the conditions while still maintaining the ability to detect
>> functional problems - eg. decreasing peak without explicitly resetting
>> it and reporting false number of threads.
>>
>> The webrev is at:
>> http://cr.openjdk.java.net/~jbachorik/8020875/webrev.00
>>
>> Thanks,
>>
>> -JB-
>>
>