RFR: 8225690: Multiple AttachListener threads can be created

[Chris Plummer]

Hi Yasumasa,

Sorry about the delay in getting back to you. I've looked at your 
updated webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.02/

Comments below:

On 7/10/19 5:18 PM, Yasumasa Suenaga wrote:
> Hi Chris,
>
> Thank you for your comment!
> I uploaded new webrev:
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.01/
>
> I write some comments the following.
>
> On 2019/07/11 4:29, Chris Plummer wrote:
>> Hi Yasumasa,
>>
>> I just took a quick look at this. I understand a little about how the 
>> attach mechanism works, but am by no means an expert, and find myself 
>> easily lost in some of the logic. I'll look again after others have 
>> also contributed comments. Here area few comments.
>>
>> 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).
>
>> How are errors in AttachListener::init() properly propagated. I see 
>> is_init_trigger() does the following:
>>
>>   548     if (os::Posix::matches_effective_uid_or_root(st.st_uid)) {
>>   549       init();
>>   550       log_trace(attach)("Attach triggered by %s", fn);
>>   551       return true;
>>
>> So "true" is returned even though init() maybe have failed, but then 
>> check_socket_file() doesn't even check the result from 
>> is_init_trigger():
>>
>>   509     is_init_trigger();
>>   510     return true;
>>
>> The SIGBREAK code does check the is_init_trigger() result, but since 
>> init() failures are not propagated to is_init_trigger(), the result 
>> may not be accurate.
>
> In case of Linux, HotSpot has some phase to start Attach Listener:
>
>   1. Check attach file (.attach_pid*)  -> is_init_trigger()
>   2. Create unix domain socket         -> is_init_trigger()
>   3. Start Attach Listener thread      -> init()
>
> is_init_trigger() returns whether init() is kicked or not.
> OTOH init() is declared as a void function, so we cannot know
> Attach Listener is started.
> (We can know it through exception message on the console, but
>  it will not handle in HotSpot.)
>
> Thus I thought we can add new field AttachListener::_state
> to know current status of Attach Listener.
>
Ok.
>
>> Have you done any testing of the error handling by forcing errors to 
>> happen?
>
> Do you mean it is the race of the attach request?
> If so, I've added the testcase (ConcAttachTest.java).
>
> 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.
>
>
>> The following code needs a comment to indicate that the current state 
>> is AL_INITIALIZED, and if the socket file was removed it needs to be 
>> recreated and a new socket opened.
>>
>>   379           } else if (AttachListener::check_socket_file()) {
>>   380             continue;
>>   381           }
>
> I added it in new webrev.

Thanks!

Chris

>
>
> Thanks,
>
> Yasumasa
>
>
>> thanks,
>>
>> Chris
>>
>>
>> On 7/4/19 6:27 AM, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> Please review this change:
>>>
>>>   JBS: https://bugs.openjdk.java.net/browse/JDK-8225690
>>>   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.00/
>>>
>>> This issue has been discussed on [1] and [2].
>>> This webrev passed tests on submit repo 
>>> (mach5-one-ysuenaga-JDK-8225690-20190704-1214-3626907).
>>>
>>> It includes the fix for JDK-8225193. They relate each other, so I 
>>> fix them together.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1] 
>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028585.html
>>> [2] 
>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-June/028418.html
>>
>>
>>