RFR: 8225690: Multiple AttachListener threads can be created

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Thu Jul 11 17:05:57 UTC 2019


Hi Yasumasa,


On 7/11/19 05:10, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> On 2019/07/11 17:06, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>>
>> On 7/10/19 23:56, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> 2019年7月11日(木) 15:23 serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com>>:
>>>
>>>     Hi Yasumasaa,
>>>
>>>     It looks good to me in general.
>>>     It is hard to prove the Attach Listener initialization state 
>>> machine is fully correct.
>>>
>>>     One comment though.
>>>
>>>     +bool AttachListener::check_socket_file() {
>>>     . . .
>>>     + is_init_trigger();
>>>     + return true;
>>>     + }
>>>     + return false;
>>>     +} I was expecting a value fromthe is_init_trigger()  call be 
>>> returned:
>>>        returnis_init_trigger(); Do I miss anything here?
>>>
>>>
>>> I expect check_socket_file() returns whether is_init_trigger() was 
>>> kicked. So I return true at this point.
>>> Should this function return the result of is_init_trigger()?
>>
>> It is not clear to me what is your intention.
>> Is it Okay for the check_socket_file() to return true even if the 
>> is_init_trigger() returned false?
>
> I'm not particular about this.
> So I uploaded new webrev which redirects the result of is_init_trigger().
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.02/
>
> The change from previous webrev:
>
>   http://hg.openjdk.java.net/jdk/submit/rev/7740b1b1f2f6

The update looks Okay to me.

Thanks,
Serguei

>
> Thanks,
>
> Yasumasa
>
>
>> Thanks,
>> Serguei
>>
>>
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>>     Thanks, Serguei
>>>
>>>
>>>
>>>     On 7/10/19 17:18, 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.
>>>>
>>>>     We might be able to use ShouldNotReachHere() at here.
>>>>
>>>>
>>>>>     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.
>>>>
>>>>
>>>>>     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.
>>>>
>>>>
>>>>>     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,
>>>>
>>>>     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
>>>>>
>>>>>
>>>>>
>>>
>>



More information about the serviceability-dev mailing list