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

[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.spitsyn at 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.spitsyn at 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
>>>
>