A Bug about the JVM Attach mechanism

[serguei.spitsyn at oracle.com]

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