RFR: 8225690: Multiple AttachListener threads can be created
Yasumasa Suenaga
yasuenag at gmail.com
Thu Jul 11 23:06:32 UTC 2019
Thanks Serguei!
Chris, how about this?
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.02/
Yasumasa
On 2019/07/12 2:05, serguei.spitsyn at oracle.com wrote:
> 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