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
