RFR (S): 8211909: JDWP Transport Listener: dt_socket thread crash

[David Holmes]

Hi Dmitry,

On 16/10/2018 10:46 PM, Dmitry Samersoff wrote:
> David,
> 
> The fix looks good to me.

Thanks for taking a look.

> PS: This code has lots of formatting nits like missed spaces.
>      It's clearly out of scope of this fix, but it might be worth to
>      launch a second webrev.

out of scope but launch a second webrev? Sorry I'm a little confused. 
But I only see a few missed space around for-loops:

929   for (int i=0; i < nthreads; i++) {
1463       for (int i=0, j=0; i<nthreads; i++) {
1495       for (int i=0; i<ngroups; i++) {
3250       for (intptr_t i=0; i <= recursion; i++) {
3680         for (int j=0; j<readable_count; j++) {

I can fix those before pushing.

Thanks,
David

> 
> -Dmitry
> 
> On 15.10.2018 8:12, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211909
>> webrev: http://cr.openjdk.java.net/~dholmes/8211909/webrev/
>>
>> The crash occurs when trying to access a thread that was returned as
>> part of JVM TI GetThreadGroupChildren. The problem is that the JVM TI
>> code tries to use the Threads_lock to ensure atomic access to the
>> ThreadGroup's threads and child groups:
>>
>>      { // Cannot allow thread or group counts to change.
>>        MutexLocker mu(Threads_lock);
>>
>> but the Threads_lock does not control concurrency in the Java code that
>> can cause threads to be added and removed, so we do not get a stable
>> snapshot of the thread array and its length, and contents. To get a
>> stable snapshot we have to use the same synchronization mechanism as
>> used by the Java code: lock the monitor of the ThreadGroup instance.
>>
>> Two other pointless acquisitions of the Threads_lock, in GetThreadInfo
>> and GetThreadGroupInfo, were also removed. These functions could report
>> an inconsistent snapshot of the Thread or ThreadGroup state, if the
>> mutable state is mutated concurrently. Again we could acquire the
>> object's monitor to prevent this but it is unclear that there is any
>> real benefit in doing so - after all the thread/group information could
>> change immediately after it has been read anyway. So here I just delete
>> the Threads_lock usage.
>>
>> Testing: mach5 tier 1-3
>>           JVM TI tests (serviceability/jvmti vmTestbase/nsk/jvmti/)
>>
>> A regression test may be possible if we call GetThreadGroupChildren in a
>> loop, whilst threads add and remove themselves from the group
>> concurrently. But it is awkward to write.
>>
>> Thanks,
>> David
>>
>> Thanks,
>> David
> 
>