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

[serguei.spitsyn at oracle.com]

Hi David and Alex,

My understanding is that previous implementation collected logs 
separately for each thread in TLS, and at the end, merged and sorted out 
the output by log id.
So, the result is that all messages are serialized at the end.
Alex changed the implementation but the result is the same - all log 
messages are serialized.

There are two tests which use the LockFreeLogger.
Another one is: test/jdk/java/lang/Thread/ThreadStateController.java .
Does the ThreadStateController.java work okay after the fix?

Thanks,
Serguei


On 3/5/20 10:54, 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));
>
> 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.
>
> --alex
>
>>
>> Thanks,
>> David
>>
>>> --alex