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

[Yasumasa Suenaga]

Hi Alex,

On 2020/05/12 9: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/

I sent the reply before reading yours :)

Looks good to me.


Thanks,

Yasumasa


> --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
>>>>>>
>>>>