Hi Serguei,
2019年7月11日(木) 15:23 serguei.spit...@oracle.com <serguei.spit...@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?
>
>
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
>
>
>
>
>
>
