Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 




On 05/11/2020 17:21, serguei.spit...@oracle.com wrote:

On 5/11/20 17:17, Alex Menkov wrote:



On 05/11/2020 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in 
is_init_trigger(), it might be affect to incorrect behavior.

So I suggested to wrap ThreadInVM and while loop with code block ({}).


I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/ 



It is okay with me in general.
But as a side opinion: placing the ThreadInVM inside the loop would be 
simpler and without any extra problems.


I'm ok with both ways.
Especially taking in mind that this case (when we need to restart attach 
listener) is quite artificial.


--alex




Thanks,
Serguei



--alex




Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding 
ThreadBlockInVM,

so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely rare 
case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test 
timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits 
while attach listener state becomes AL_NOT_INITIALIZED (it 
happens when AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are 
used by 2 threads (attach listener thread and signal handler 
thread) without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

On 5/11/20 17:33, Yasumasa Suenaga wrote:

On 2020/05/12 9:17, serguei.spit...@oracle.com wrote:

Hi Yasumasa,


On 5/11/20 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in 
is_init_trigger(), it might be affect to incorrect behavior.

So I suggested to wrap ThreadInVM and while loop with code block ({}).


Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.


I'm not sure that.

The attach listener state would be transit to AL_NOT_INITIALIZED when 
AttachListener::dequeue() returns NULL.
listener_cleanup() would shoutdown the socket, then accept() would be 
return with error at LinuxAttachListener::dequeue() in case of Linux.


If they performs on high-spec (multi-core, and high-frequency) 
machine, while loop might not be finished with one iteration.

It might be long way between shutdown() call and state transition.

I agree with you this problem is rare case, so it is not a big problem 
ThreadInVM is located to inside the loop.

But I prefer to locate it to outside the loop.


I'm okay with both approaches as the differences are minor and potential 
performance issue (if any) is very minor.

My normal preferences are in favor of simplicity. :)

Thanks,
Serguei



Thanks,

Yasumasa



I do not see any potential performance problem here.

Thanks,
Serguei


Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at 
safepoint

537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding 
ThreadBlockInVM,

so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely 
rare case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test 
timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is 
closed.
I don't think this _shutdown flag helps a lot, but I don't want 
to make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits 
while attach listener state becomes AL_NOT_INITIALIZED (it 
happens when AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are 
used by 2 threads (attach listener thread and signal handler 
thread) without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Yasumasa Suenaga 

Hi Alex,

On 2020/05/12 9:17, Alex Menkov wrote:



On 05/11/2020 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a thread 
"_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in is_init_trigger(), it 
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).


I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/


I sent the reply before reading yours :)

Looks good to me.


Thanks,

Yasumasa



--alex




Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539  AL_NOT_INITIALIZED) != 
AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.

Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/

On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
 + // if for a reason the app hangs, we don't want to wait test timeout

Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html

Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make 
significant changes in the AIX code as I cannot test it.

--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/

Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach 
listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.

Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads 
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for 
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Yasumasa Suenaga 

On 2020/05/12 9:17, serguei.spit...@oracle.com wrote:

Hi Yasumasa,


On 5/11/20 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a thread 
"_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in is_init_trigger(), it 
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).


Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.


I'm not sure that.

The attach listener state would be transit to AL_NOT_INITIALIZED when 
AttachListener::dequeue() returns NULL.
listener_cleanup() would shoutdown the socket, then accept() would be return 
with error at LinuxAttachListener::dequeue() in case of Linux.

If they performs on high-spec (multi-core, and high-frequency) machine, while 
loop might not be finished with one iteration.
It might be long way between shutdown() call and state transition.

I agree with you this problem is rare case, so it is not a big problem 
ThreadInVM is located to inside the loop.
But I prefer to locate it to outside the loop.


Thanks,

Yasumasa



I do not see any potential performance problem here.

Thanks,
Serguei


Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.

Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/

On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
 + // if for a reason the app hangs, we don't want to wait test timeout

Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html

Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make 
significant changes in the AIX code as I cannot test it.

--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/

Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach 
listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.

Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads 
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for 
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 




On 05/11/2020 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in 
is_init_trigger(), it might be affect to incorrect behavior.

So I suggested to wrap ThreadInVM and while loop with code block ({}).


I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/

--alex




Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539  AL_NOT_INITIALIZED) != 
AL_NOT_INITIALIZED) {

  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely rare 
case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

On 5/11/20 17:17, Alex Menkov wrote:



On 05/11/2020 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in 
is_init_trigger(), it might be affect to incorrect behavior.

So I suggested to wrap ThreadInVM and while loop with code block ({}).


I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/


It is okay with me in general.
But as a side opinion: placing the ThreadInVM inside the loop would be 
simpler and without any extra problems.


Thanks,
Serguei



--alex




Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding 
ThreadBlockInVM,

so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely rare 
case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test 
timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits 
while attach listener state becomes AL_NOT_INITIALIZED (it 
happens when AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are 
used by 2 threads (attach listener thread and signal handler 
thread) without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

Hi Yasumasa,


On 5/11/20 17:08, Yasumasa Suenaga wrote:

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in 
is_init_trigger(), it might be affect to incorrect behavior.

So I suggested to wrap ThreadInVM and while loop with code block ({}).


Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.
I do not see any potential performance problem here.

Thanks,
Serguei


Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely rare 
case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test 
timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits 
while attach listener state becomes AL_NOT_INITIALIZED (it 
happens when AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are 
used by 2 threads (attach listener thread and signal handler 
thread) without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 

Updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/

--alex


On 05/11/2020 14:12, Alex Menkov wrote:

Please hold on with the review.

webrev.2 causes LingeredApp crash.
Looks like need to decrease ThreadBlockInVM scope.
Will send new version after completing test run.

On 05/11/2020 13:52, Chris Plummer wrote:


  228 // If the app hangs, we don't want to wait for the 
to test timeout.


Sorry, there was a typo in my suggestion. Should be "test to", not "to 
test".


Oh, I didn't pay enough attention to comment updates :)

--alex



thanks,

Chris

On 5/11/20 12:53 PM, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Yasumasa Suenaga 

On 2020/05/12 9:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a thread 
"_thread_blocked" VM does not need to suspend it at safepoint.


I think so, but if state would be transit to the other in is_init_trigger(), it 
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).


Thanks,

Yasumasa



--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539  AL_NOT_INITIALIZED) != 
AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.

Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/

On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
 + // if for a reason the app hangs, we don't want to wait test timeout

Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html

Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make 
significant changes in the AIX code as I cannot test it.

--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/

Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach 
listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.

Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads 
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for 
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

Hi Alex,

I see the point, thanks.
Serguei


On 5/11/20 17:01, Alex Menkov wrote:

Hi Serguei,

If I understand correctly, this also should work - when we mark a 
thread "_thread_blocked" VM does not need to suspend it at safepoint.


--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.
Also, we do not care about performance here as it is extremely rare 
case.


Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener 
restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 

Hi Serguei,

If I understand correctly, this also should work - when we mark a thread 
"_thread_blocked" VM does not need to suspend it at safepoint.


--alex

On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:

Hi Alex,

I'm not sure this update suggested by Yasumasa is right:

536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
  538 while (AttachListener::transit_state(AL_INITIALIZING,
  539  AL_NOT_INITIALIZED) != 
AL_NOT_INITIALIZED) {
  540   os::naked_yield();
  541 }


The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each 
iteration.

Also, we do not care about performance here as it is extremely rare case.

Thanks,
Serguei


On 5/11/20 12:53, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

  
  
Hi Alex,
  
  I'm not sure this update suggested by Yasumasa is right:
   536 // avoid deadlock if AttachListener thread is blocked at safepoint
 537 ThreadBlockInVM tbivm(JavaThread::current()); 
 538 while (AttachListener::transit_state(AL_INITIALIZING,
 539  AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
 540   os::naked_yield();
 541 }
  
  The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at
each iteration.
Also, we do not care about performance here as it is extremely
rare case. 
  
  Thanks,
  Serguei
  
  
  On 5/11/20 12:53, Alex Menkov wrote:

Hi
  Yasumasa, Serguei, Chris,
  
  
  Thank you for the feedback
  
  updated webrev (all suggestions are applied):
  
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
  
  
  On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
  
  Hi Alex,


It looks good in general.

I have a couple of minor comments.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout


Nit: replace: wait test timeout => wait for test timeout


I hope, you remember about copyright comments update.



http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html


Q1: How useful is this variable? :

AixAttachListener::_shutdown = false;


 Why is it needed on aix but not on other platforms?

  
  
  IFAIU AIX has issue with accept() - it hangs if the socket is
  closed.
  
  I don't think this _shutdown flag helps a lot, but I don't want to
  make significant changes in the AIX code as I cannot test it.
  
  
  --alex
  
  
  

Thanks,

Serguei



On 5/8/20 18:14, Alex Menkov wrote:

Hi all,
  
  
  please review the fix for
  
  https://bugs.openjdk.java.net/browse/JDK-8235211
  
  webrev:
  
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
  
  
  Test failures are caused by deadlock during attach listener
  restarting:
  
  check_socket_file function aborts socket listening and waits
  while attach listener state becomes AL_NOT_INITIALIZED (it
  happens when AttachListener::dequeue returns NULL).
  
  AttachListener::dequeue method is blocked in ThreadBlockInVM
  dtor.
  
  To solve it ThreadBlockInVM was added inside waiting cycle in
  check_socket_file.
  
  
  Other changes:
  
  - made _listener (and _shutdown for aix) volatile as they are
  used by 2 threads (attach listener thread and signal handler
  thread) without synchronization;
  
  - added close() for listening socket on aix (before it had
  only shutdown() for it);
  
  - additional logging and some cleanup in the test;
  
  - added handling of LingeredApp hang.
  
  
  --alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 

Please hold on with the review.

webrev.2 causes LingeredApp crash.
Looks like need to decrease ThreadBlockInVM scope.
Will send new version after completing test run.

On 05/11/2020 13:52, Chris Plummer wrote:


  228 // If the app hangs, we don't want to wait for the to 
test timeout.


Sorry, there was a typo in my suggestion. Should be "test to", not "to 
test".


Oh, I didn't pay enough attention to comment updates :)

--alex



thanks,

Chris

On 5/11/20 12:53 PM, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Chris Plummer 



 228 // If the app hangs, we don't want to wait for the to 
test timeout.


Sorry, there was a typo in my suggestion. Should be "test to", not "to 
test".


thanks,

Chris

On 5/11/20 12:53 PM, Alex Menkov wrote:

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ 



On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html 



Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

 Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to 
make significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used 
by 2 threads (attach listener thread and signal handler thread) 
without synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Alex Menkov 

Hi Yasumasa, Serguei, Chris,

Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/

On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html 
+ // if for a reason the app hangs, we don't want to wait test timeout


Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html

Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;

     Why is it needed on aix but not on other platforms?


IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make 
significant changes in the AIX code as I cannot test it.


--alex



Thanks,
Serguei


On 5/8/20 18:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 
2 threads (attach listener thread and signal handler thread) without 
synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

  
  
I do not see my message reached the
  serviceability-dev mailing list.
  Resending...
  
  
  On 5/11/20 00:31, serguei.spit...@oracle.com wrote:


  Hi Alex,

It looks good in general.
I have a couple of minor comments.

http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

+// if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
  
  I hope, you remember about copyright comments update.
  
  
  http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
 
Q1: How useful is this variable? :
    AixAttachListener::_shutdown = false;
  
      Why is it needed on aix but not on other platforms?
  
  Thanks,
  Serguei
  

On 5/8/20 18:14, Alex Menkov wrote:
  
  Hi
all, 

please review the fix for 
https://bugs.openjdk.java.net/browse/JDK-8235211

webrev: 
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/


Test failures are caused by deadlock during attach listener
restarting: 
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL). 
AttachListener::dequeue method is blocked in ThreadBlockInVM
dtor. 
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file. 

Other changes: 
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization; 
- added close() for listening socket on aix (before it had only
shutdown() for it); 
- additional logging and some cleanup in the test; 
- added handling of LingeredApp hang. 

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread Chris Plummer 

Hi Alex,

 228 // if for a reason the app hangs, we don't want to 
wait test timeout
 229 if 
(!appProcess.waitFor(Utils.adjustTimeout(appWaitTime), TimeUnit.SECONDS)) {


Can you fix the comment. Maybe:

 228 // If the app hangs, we don't want to wait for the to 
test timeout.


And your use of appWaitTime had me look for other uses, and I noticed a 
pre-existing comment there that could use some work. Perhaps you can 
clean it up with this RFR.


 273  * Waits the application to start with the default timeout.

Should be "Waits for the application..."

thanks,

Chris

On 5/8/20 6:14 PM, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ 



Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 
2 threads (attach listener thread and signal handler thread) without 
synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-11 Thread serguei.spit...@oracle.com 

  
  
Hi Alex,
  
  It looks good in general.
  I have a couple of minor comments.
  
  http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html

+// if for a reason the app hangs, we don't want to wait test timeout
  Nit: replace: wait test timeout => wait for test timeout

I hope, you remember about copyright comments update.


http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
  
  
  Q1: How useful is this variable? :
      AixAttachListener::_shutdown = false;

    Why is it needed on aix but not on other platforms?

Thanks,
Serguei

  
  On 5/8/20 18:14, Alex Menkov wrote:

Hi all,
  
  
  please review the fix for
  
  https://bugs.openjdk.java.net/browse/JDK-8235211
  
  webrev:
  
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
  
  
  Test failures are caused by deadlock during attach listener
  restarting:
  
  check_socket_file function aborts socket listening and waits while
  attach listener state becomes AL_NOT_INITIALIZED (it happens when
  AttachListener::dequeue returns NULL).
  
  AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
  
  To solve it ThreadBlockInVM was added inside waiting cycle in
  check_socket_file.
  
  
  Other changes:
  
  - made _listener (and _shutdown for aix) volatile as they are used
  by 2 threads (attach listener thread and signal handler thread)
  without synchronization;
  
  - added close() for listening socket on aix (before it had only
  shutdown() for it);
  
  - additional logging and some cleanup in the test;
  
  - added handling of LingeredApp hang.
  
  
  --alex

Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-09 Thread Yasumasa Suenaga 

Hi Alex,

It seems to be hanged in check_socket_file() because AttachListener::deque() 
could not return due to safepoint.
So it is good idea to use ThreadBlockInVM, but I think we can change as below 
to reduce overhead:

```
{
  ThreadBlockInVM tbivm(JavaThread::current());
  while (AttachListener::transit_state(AL_INITIALIZING,
   AL_NOT_INITIALIZED) != 
AL_NOT_INITIALIZED) {
os::naked_yield();
  }
}
```

It might consume much CPU time with long safepoint, but I think it is corner 
case because this issue occurs intermittently.


Thanks,

Yasumasa


On 2020/05/09 10:14, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/

Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach 
listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.

Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads 
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for 
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex

RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file

2020-05-08 Thread Alex Menkov 

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/

Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while 
attach listener state becomes AL_NOT_INITIALIZED (it happens when 
AttachListener::dequeue returns NULL).

AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in 
check_socket_file.


Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 
threads (attach listener thread and signal handler thread) without 
synchronization;
- added close() for listening socket on aix (before it had only 
shutdown() for it);

- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.

--alex