[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8225690: Multiple AttachListener threads can be created
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2019-07-15 17:44:45
Message-ID: 38008946-0674-4c88-ea08-db78996659c9 () oracle ! com
[Download RAW message or body]

Hi Yasumasa,

Looks good. Thanks for the extra testing.

Chris

On 7/15/19 5:48 AM, Yasumasa Suenaga wrote:
> Hi Chris,
>
> I uploaded new webrev:
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.03/
>
> On 2019/07/15 15:48, Chris Plummer wrote:
>
> ...snip...
>
>>>> I don't understand the need for the following in 
>>>> attach_listener_thread_entry()
>>>>
>>>>     428 AttachListener::set_state(AL_NOT_INITIALIZED);
>>>>
>>>> There is no path to this statement since the only way out of the 
>>>> loop is the following:
>>>
>>> I agree with you, but I think we need to reset the status of Attach 
>>> Listener
>>> in the end of the function.
>> Why, if it is never executed?
>>>
>>> We might be able to use ShouldNotReachHere() at here.
>>>
>> I think ShouldNotReachHere() would be appropriate at the end of this 
>> function, but it's really just adding an assert for what I've already 
>> said is the case. You can't reach this point so there is no reason to 
>> add any logic there (other than asserting).
>
> I replaced it to ShouldNotReachHere() in new webrev.
>
>
> ...snip...
>
>
>>> If you mean the failure of init() and/or is_init_trigger(),
>>> I do not have idea how to test it.
>> Yes, I mean unexpected failures that you've written code to handle. 
>> The best way to test them is either in gdb or just programmatically 
>> introduce the failure (change the code so the failure happens). I'm 
>> just asking that you step through the code one time under failure, 
>> not that you have a test to induce the failure, which probably is not 
>> possible.
>
> I tested this patch with the change to return false immediately in 
> is_init_trigger().
> It works fine with ConcAttachTest.java . LingeredApp did not create 
> Attach Listener thread, and also it did not hang.
>
>
> Thanks,
>
> Yasumasa



[prev in list] [next in list] [prev in thread] [next in thread]