Re: A Bug about the JVM Attach mechanism

[serguei.spit...@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