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