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

[serguei.spitsyn at oracle.com]

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 do not see any potential performance problem here.

Thanks,
Serguei

> Thanks,
>
> Yasumasa
>
>
>> --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
>>>>>
>>>