RFR: JDK-8240340: java/lang/management/ThreadMXBean/Locks.java is buggy

[David Holmes]

Hi Alex,

On 6/03/2020 4:54 am, Alex Menkov wrote:
> Hi David,
> 
> Thanks you for the review.
> 
> On 03/04/2020 17:50, David Holmes wrote:
>> Hi Alex,
>>
>> On 5/03/2020 10:30 am, Alex Menkov wrote:
>>> Hi all,
>>>
>>> please review the fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8240340
>>> webrev:
>>> http://cr.openjdk.java.net/~amenkov/jdk15/ThreadMXBean_Locks_test/webrev/ 
>>>
>>>
>>> changes:
>>> - assertThreadState method: don't re-read thread state throwing 
>>> exception (as we got weird error like "Thread WaitingThread is at 
>>> WAITING state but is expected to be in Thread.State = WAITING");
>>> - added proper test shutdown on error (made all threads "daemon", 
>>> interrupt waiting thread if CheckerThread throws exception);
>>> - if CheckerThread detects error, propagate the exception to main 
>>> thread;
>>
>> The test changes seem fine.
>>
>>> - fixed LockFreeLogger class - it should work for logging from 
>>> several threads, but it doesn't. I prefer to simplify it just to keep 
>>> ConcurrentLinkedQueue<String>.
>>> LockFreeLogger is also used by ThreadMXBeanStateTest test, but only 
>>> by a single thread.
>>
>> I don't understand your changes here as you've completely changed the 
>> intended design of the logger. The original accumulates log entries 
>> per-thread and then spits them all out (though I'm not clear on the 
>> exact ordering - I don't how to read that stream stuff). The new code 
>> just creates a single queue of log records interleaving entries from 
>> different threads. The simple logger may be all that is needed but it 
>> seems quite different to the intent of the original.
> 
> Testing changes in the test I discovered that there is something wrong 
> with the logger - it printed only part of the records, so I have to look 
> at the LockFreeLogger class and I don't understand how it was supposed 
> to work.
> About ordering in cumulative log: each record has Integer which used to 
> sort log entries from all threads (i.e. records from different threads 
> are printed at the order which log() was called).
> Looking at allRecords/records stuff I don't understand how it should be 
> used. To get logs from different threads in one logger, we needs one 
> instance. So we create LockFreeLogger (in main thread) and ctor creates 
> ThreadLocal record and register it in allRecords. Logging from main 
> thread works fine, but if any other thread tries to log, 1st log() call 
> creates its own ThreadLocal records (by records.get()) and log records 
> from this thread go there. But this ThreadLocal records is not 
> registered in allRecords, so this logging won't be included in final log.
> Looks like we need to change log() to something like
> 
> Map<Integer, String> recs = records.get();
> if (recs.isEmpty()) {
>      allRecords.add(recs);
> }
> recs.put(id, String.format(format, params));

Yep good catch - this logger was completely broken.

> But all this stuff do exactly the same as simple ConcurrentLinkedQueue 
> (i.e. lock free ordered list).
> At least I don't see other rationale in the stuff.

I'm not certain of intent with the original but I'd always want to see 
log entries in chronological order - which is what we now clearly have.

Thanks,
David

> --alex
> 
>>
>> Thanks,
>> David
>>
>>> --alex