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

[Alex Menkov]

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