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

[David Holmes]

Looks good.

Thanks,
David

On 10/03/2020 5:15 am, Alex Menkov wrote:
> 
> Updated webrev:
> http://cr.openjdk.java.net/~amenkov/jdk15/ThreadMXBean_Locks_test/webrev.02/ 
> 
> 
> Changes are in LockFreeLogger comments only.
> 
> --alex
> 
> On 03/08/2020 21:19, David Holmes wrote:
>> P.S.
>>
>> Forgot to note however that you need to update the documentation for 
>> the logger now as the mention of "per-thread logs" makes no sense now. 
>> Also in the spirit of not using @author, and because this is no longer 
>> the code created by Jaroslav, please delete the @author line.
>>
>> Thanks,
>> David
>>
>> On 9/03/2020 2:15 pm, David Holmes wrote:
>>> 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