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
