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

[Alex Menkov]

On 05/11/2020 17:21, serguei.spitsyn at 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.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
>>>>>>>
>>>>>
>