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

[serguei.spitsyn at oracle.com]

On 5/11/20 17:33, Yasumasa Suenaga wrote:
> On 2020/05/12 9:17, serguei.spitsyn at oracle.com wrote:
>> 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'm not sure that.
>
> The attach listener state would be transit to AL_NOT_INITIALIZED when 
> AttachListener::dequeue() returns NULL.
> listener_cleanup() would shoutdown the socket, then accept() would be 
> return with error at LinuxAttachListener::dequeue() in case of Linux.
>
> If they performs on high-spec (multi-core, and high-frequency) 
> machine, while loop might not be finished with one iteration.
> It might be long way between shutdown() call and state transition.
>
> I agree with you this problem is rare case, so it is not a big problem 
> ThreadInVM is located to inside the loop.
> But I prefer to locate it to outside the loop.

I'm okay with both approaches as the differences are minor and potential 
performance issue (if any) is very minor.
My normal preferences are in favor of simplicity. :)

Thanks,
Serguei


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