A Bug about the JVM Attach mechanism

[Yasumasa Suenaga]

Hi Serguei,

Can I send RFR for JDK-8225690 and close JDK-8225193 as duplicate of it?


Thanks,

Yasumasa


On 2019/07/03 1:06, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
> 
> The approach looks Okay in general.
> Could you, please, post an RFR with the bug number in the email subject?
> 
> Thanks,
> Serguei
> 
> 
> On 6/26/19 18:41, Yasumasa Suenaga wrote:
>> Hi Serguei,
>>
>> Thank you for your comment.
>> I uploaded new webrev which includes the fix.
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.3/
>>
>> Also I added TODO for other Un*x OSes.
>> If the change for Linux is OK, I will copy it to others.
>> (I do not have them, so I depend on submit repo.)
>>
>> This webrev has passed test/hotspot/jtreg/serviceability/attach and
>> test/jdk/com/sun/tools/attach jtreg tests on Linux x64.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>> 2019年6月27日(木) 6:10 serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>:
>>> Hi Yasumasa,
>>>
>>> I'm reviewing it.
>>>
>>> Just a quick comment.
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html
>>>
>>> +  // reads a request from the given connected socket
>>> +  static LinuxAttachOperation* read_request(int s);
>>> +
>>> +  static bool _atexit_registered;
>>> +
>>> + public:
>>> +  enum {
>>> +    ATTACH_PROTOCOL_VER = 1                     // protocol version
>>> +  };
>>> +  enum {
>>> +    ATTACH_ERROR_BADVERSION     = 101           // error codes
>>> +  };
>>> +
>>>     static void set_path(char* path) {
>>>       if (path == NULL) {
>>> +      _path[0] = '\0';
>>>         _has_path = false;
>>>       } else {
>>>         strncpy(_path, path, UNIX_PATH_MAX);
>>>         _path[UNIX_PATH_MAX-1] = '\0';
>>>         _has_path = true;
>>>       }
>>>     }
>>>
>>>     static void set_listener(int s)               { _listener = s; }
>>>
>>> -  // reads a request from the given connected socket
>>> -  static LinuxAttachOperation* read_request(int s);
>>> -
>>> - public:
>>> -  enum {
>>> -    ATTACH_PROTOCOL_VER = 1                     // protocol version
>>> -  };
>>> -  enum {
>>> -    ATTACH_ERROR_BADVERSION     = 101           // error codes
>>> -  };
>>> -
>>>
>>>
>>> You moved public definitions of enums up in the code.
>>> All the declarations below that were private before became public now.
>>> Not sure, if it was your intention
>>>
>>> Also, the Copyright comment in this file needs an update.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>> On 6/26/19 07:34, Yasumasa Suenaga wrote:
>>>
>>> Hi,
>>>
>>>>> I'm not clear how this addresses the issue with deleting the file? The
>>>>> file still has to exist IIUC for the mechanism to work.
>>>>>
>>>>> This seems more suited to fixing the "multiple attach threads" problem.
>>> How about this change?
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/
>>>
>>> I think it can fix both JDK-8225690 and JDK-8225193.
>>>
>>> This webrev implements for Linux only.
>>> If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/06/21 23:16, Yasumasa Suenaga wrote:
>>>
>>> On 2019/06/21 22:51, David Holmes wrote:
>>>
>>> Hi Yasumasa,
>>>
>>> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>>>
>>> Hi,
>>>
>>> Can we fix this issue like this webrev?
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>>>
>>>
>>> I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.
>>>
>>> This seems more suited to fixing the "multiple attach threads" problem.
>>>
>>>
>>> Yes, my proposal focuses to "multiple attach threads".
>>> I shared my patch because it might help the work for this issue.
>>>
>>> This patch would guard multithread-issue in Attach Listener.
>>> So unix domain socket file will create just once.
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> David
>>>
>>> I could reproduce this issue with ConcAttachTest.java in it
>>> on my laptop.
>>> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>>>
>>> If we need to fix based on current implementation, I think
>>> we need to use atomic operation.
>>> But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/06/21 5:49, David Holmes wrote:
>>>
>>> Sorry it took me a while to understand the specifics of the problem. :)
>>>
>>> David
>>>
>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>
>>> Yes Alan, I mean this
>>> ------------------?Original?------------------
>>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>>> Holmes"<david.holmes at oracle.com>;
>>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>>> On 20/06/2019 05:10, nijiaben wrote:
>>>    > :
>>>    > I know this mechanism, can we provide means of recovery to avoid
>>> unavailability caused by accidental deletion?
>>>    >
>>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>>> /tmp? There may indeed be an issue for applications that run for weeks
>>> or months. If someone is using jmap, jcmd or other tools using the
>>> attach API then it will trigger the attach listener to start. When they
>>> come back in a few weeks then the .java_pid<pid> file may have been
>>> removed so they cannot attach. Is this what what you are pointing out?
>>>
>>> -Alan
>>>
>>>
>