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.spit...@oracle.com <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 > >
