RFR: 8225690: Multiple AttachListener threads can be created

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Chris Plummer
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:

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.

Have you done any testing of the error handling by forcing errors to happen?

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           }

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



Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
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
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

serguei.spitsyn@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 from the is_init_trigger() call be returned:
  return is_init_trigger();

Do I miss anything here?

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




Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
Hi Serguei,

2019年7月11日(木) 15:23 [hidden email] <[hidden email]>:
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 from the is_init_trigger() call be returned:
  return is_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()?


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




Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

serguei.spitsyn@oracle.com
Hi Yasumasa,


On 7/10/19 23:56, Yasumasa Suenaga wrote:
Hi Serguei,

2019年7月11日(木) 15:23 [hidden email] <[hidden email]>:
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 from the is_init_trigger() call be returned:
  return is_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?

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





Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
Hi Serguei,

On 2019/07/11 17:06, [hidden email] wrote:

> Hi Yasumasa,
>
>
> On 7/10/19 23:56, Yasumasa Suenaga wrote:
>> Hi Serguei,
>>
>> 2019年7月11日(木) 15:23 [hidden email] <mailto:[hidden email]> <[hidden email] <mailto:[hidden email]>>:
>>
>>     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


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
>>>>
>>>>
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

serguei.spitsyn@oracle.com
Hi Yasumasa,


On 7/11/19 05:10, Yasumasa Suenaga wrote:

> Hi Serguei,
>
> On 2019/07/11 17:06, [hidden email] wrote:
>> Hi Yasumasa,
>>
>>
>> On 7/10/19 23:56, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> 2019年7月11日(木) 15:23 [hidden email]
>>> <mailto:[hidden email]> <[hidden email]
>>> <mailto:[hidden email]>>:
>>>
>>>     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
>>>>>
>>>>>
>>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
Thanks Serguei!

Chris, how about this?

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


Yasumasa


On 2019/07/12 2:05, [hidden email] wrote:

> Hi Yasumasa,
>
>
> On 7/11/19 05:10, Yasumasa Suenaga wrote:
>> Hi Serguei,
>>
>> On 2019/07/11 17:06, [hidden email] wrote:
>>> Hi Yasumasa,
>>>
>>>
>>> On 7/10/19 23:56, Yasumasa Suenaga wrote:
>>>> Hi Serguei,
>>>>
>>>> 2019年7月11日(木) 15:23 [hidden email] <mailto:[hidden email]> <[hidden email] <mailto:[hidden email]>>:
>>>>
>>>>     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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Chris Plummer
In reply to this post by Yasumasa Suenaga-4
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
>>
>>
>>


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

serguei.spitsyn@oracle.com
Hi Yasumasa,

The update looks good.

Thanks,
Serguei


On 7/15/19 05:48, 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8225690: Multiple AttachListener threads can be created

Chris Plummer
In reply to this post by Yasumasa Suenaga-4
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