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

[Alex Menkov]

On 03/06/2020 23:28, serguei.spitsyn at oracle.com wrote:
> 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?

ThreadStateController is an utility class used only by 
ThreadMXBeanStateTest.java.
ThreadMXBeanStateTest.java is problem-listed, but I verified that 
logging works in the test.

--alex


> 
> 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
>