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