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



Reply via email to