[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: A Bug about the JVM Attach mechanism
From:       Yasumasa Suenaga <yasuenag () gmail ! com>
Date:       2019-06-27 1:41:06
Message-ID: CAGFVN2AoBQ-nuSg8zg3Vc2yObTBGG0+rXVnrWS9TsYEOAdz0zw () mail ! gmail ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]