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

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