Protocol version of Attach API
David Holmes
david.holmes at oracle.com
Thu Feb 28 07:15:59 UTC 2019
Hi Lin,
On 28/02/2019 4:49 pm, 臧琳 wrote:
> Hi David,
> Your are right and thanks for pointing it out. when I worte that patch, I was considering implement -filename and -incremental together. and I must be too stupid to forget recover it when I divided the patch into two.
> And it seems a good solution is to refine the original patch of jmap histo, and try to composite all args as one when passing it to socket and let attachlistener to handle the analyze.
> I will try that.
> One more, do I need to consider changing the jmap -dump also?
I'm assuming -dump already works fine, so I'm just expecting to see
-histo handle the file in a similar manner.
If you find this works I suggest creating a sub-task of 8215622 to first
backout the original changes (hg backout), and a second sub-task to REDO
with the new implementation. Each will need reviewing separately in
their own RFR thread.
Thanks,
David
> BRs,
> Lin
> ________________________________________
> From: David Holmes <david.holmes at oracle.com>
> Sent: Thursday, February 28, 2019 12:59:28 PM
> To: 臧琳; Yasumasa Suenaga
> Cc: serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net
> Subject: Re: Protocol version of Attach API
>
> Sorry I'm going to pick up on the rollback and re-do option here as I
> just had a closer look at jmap. Given jmap -dump already has more
> options than -histo does, why was any change to the "maximum number of
> args" needed in the first place ???
>
> David
>
> On 28/02/2019 2:43 pm, David Holmes wrote:
>> Hi everyone,
>>
>> I'm not sure we're converging on a suitable solution here, but to
>> address the issues flagged by Lin below ...
>>
>> On 28/02/2019 12:39 pm, 臧琳 wrote:
>>> Hi Suenaga,
>>>
>>> Thanks for your expaination about the arg_length_max, I
>>> generally agree with you that it is better to consider using dynamic
>>> memory, and that would be handled carefully to aviod introduce
>>> compatibility issue, plus it would be a big change. So let’s see what
>>> others suggest.
>>>
>>> Hi All,
>>>
>>> It seems for me that there are basically three problems forked by this
>>> thread：
>>>
>>> ·Compatibility issue with old jcmd alike tools with attachListener’s
>>> change.
>>
>> This is issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8219721
>>
>>> ·Only 3 arguments limited to passed by socket to attachListener for
>>> Windows, which cause 8215622 work abnormally on Windows.
>>
>> I have filed a new bug for this:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8219895
>>
>>> ·The arg_length_max may not be enough for handling filename.
>>
>> I have filed a new bug for this:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8219896
>>
>> though it seems very related to this issue.
>>
>>> So I suggest we keep the first problem discussed in this thread, and
>>> create separate thread for the others. Do you agree?
>>
>> There is some overlap but yes this can be broken down somewhat - though
>> dealing with the variable length "packet" is going to have to consider
>> that what is received is in fact much larger than the purported maximum
>> packet size if these long paths are expected and accepted.
>>
>> FWIW I don't see crashes or anything drastic if the arguments are too
>> long - the operations just fail (in somewhat obscure ways sometimes).
>>
>>>
>>> For me, I will refine my patch to use timeout as a fix for the first
>>> one, and update it in this thread. And I will try to fix the second
>>> one for Windows, and create a separate thread for discussing. And if
>>> possible, I can help to fix the third one.
>>>
>>> What’s your opinion?
>>
>> That sounds fine ...
>>
>> Or you could choose to rollback JDK-8215622 and see how to solve that
>> without increasing the arg count. Given this usage:
>>
>> jmap -histo:live,file=foo.txt <pid>
>>
>> I'm not sure why this is sent to the VM as multiple args rather than as
>> a single composite arg that can then be parsed again by the actual
>> "jmap" logic. There would be some double-up perhaps if the front-end
>> tool wants to perform the command-line validation, but it would be easy
>> enough I think to do that checking then send the original composite arg.
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> BRs,
>>>
>>> Lin
>>>
>>> *From:*Yasumasa Suenaga <yasuenag at gmail.com>
>>> *Sent:* Thursday, February 28, 2019 8:39 AM
>>> *To:* 臧琳<zanglin5 at jd.com>
>>> *Cc:* David Holmes <david.holmes at oracle.com>;
>>> serviceability-dev at openjdk.java.net
>>> serviceability-dev at openjdk.java.net <serviceability-dev at openjdk.java.net>
>>> *Subject:* Re: Protocol version of Attach API
>>>
>>> 2019年2月28日(木) 0:04 臧琳 <zanglin5 at jd.com <mailto:zanglin5 at jd.com>>:
>>>
>>> Dear Suenaga,
>>> Thanks for your reviewing. I will try to refine the patch.
>>> For the argument length you mentioned, do you mean the
>>> "arg_length_max" should be large enough to accept the max filename
>>> length?
>>>
>>> Yes, but it is not enough.
>>>
>>> For example, jcmd VM.log might pass 2 or more paths to define logs.
>>>
>>> IMHO, all the handling of the argument length is at receiver
>>> side in the attachListener, such as
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/os/linux/attachListener_linux.cpp#l322,
>>> for me it means that the VM side limits the argments length less
>>> than arg_length_max, otherwise it will return NULL, which cause the
>>> sender side (tools like jcmd and jmap) exits with error message. so
>>> I think there may be no need to limit the argument size in tool side.
>>>
>>> IMHO all programs which use filesystem should support any locations on
>>> it.
>>>
>>> So I think we should use dynamic memory (or GrowableArray) for it if
>>> we do not change client side for compatibility.
>>>
>>> And from my experiment with jmap, the arguments send to
>>> sockets are not arg0 only. as you can see in
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java#l193
>>>
>>> and
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java#l133,
>>>
>>> jmap can pass arg0 as "filename", and arg1 as "-live", and both of
>>> them can be NULL. so <ver>0<cmd>0<arg>0<arg>0<arg>0 can be
>>> <ver>0<jmap>0<filename>0<live>0, and file can be null. so 00 may not
>>> indicate it reach the end.
>>>
>>> We should consider for other tools - jstack and jinfo.
>>>
>>> (jstack is ok because it will not have long arguments)
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> BRs,
>>> Lin
>>> ________________________________________
>>> From: Yasumasa Suenaga <yasuenag at gmail.com
>>> <mailto:yasuenag at gmail.com>>
>>> Sent: Wednesday, February 27, 2019 8:10:14 PM
>>> To: 臧琳
>>> Cc: David Holmes; serviceability-dev at openjdk.java.net
>>> <mailto:serviceability-dev at openjdk.java.net>
>>> Subject: Re: Protocol version of Attach API
>>>
>>> Hi Lin,
>>>
>>> I think we need to research more about this.
>>> IMHO we need to match length of arguments between
>>> server (AttachListener) and client (serviceability tools) at least.
>>> (please see previous email from me).
>>>
>>> I have some comments for your change:
>>>
>>> On 2019/02/27 18:22, 臧琳 wrote:
>>> > Dear All,
>>> > Here I have figured out one solution based on timeout. would
>>> you like help to see whether this is OK?
>>> > --- a/src/hotspot/os/linux/attachListener_linux.cpp Tue Feb
>>> 26 14:57:23 2019 +0530
>>> > +++ b/src/hotspot/os/linux/attachListener_linux.cpp Wed Feb
>>> 27 17:21:48 2019 +0800
>>> > @@ -263,9 +263,29 @@
>>> > int off = 0;
>>> > int left = max_len;
>>> >
>>> > + memset(buf, 0, max_len);
>>> > + // set timeout for read
>>> > + struct timeval timeout;
>>> > + timeout.tv_sec = 3;
>>> > + timeout.tv_usec = 0;
>>>
>>> I think timeout value should be configurable.
>>> For example, we can introduce new flag in globals.hpp .
>>>
>>>
>>> > + if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, (struct
>>> timeval*)&timeout, sizeof(timeout))) {
>>> > + log_debug(attach)("Failed to set socket option SO_RCVTIMEO:
>>> %s\n", strerror(errorno));
>>> > + printf("Warning: Failed to set socket option SO_RCVTIMEO:
>>> %s!!!\n", strerror(errno));
>>>
>>> You should not use printf(), and do you need to pass '\n' to
>>> log_debug()?
>>>
>>>
>>> > + }
>>> > do {
>>> > int n;
>>> > - RESTARTABLE(read(s, buf+off, left), n);
>>> > + log_debug(attach)("start reading data from socket....\n");
>>> > + n = read(s, buf+off, left);
>>>
>>> You should use RESTARTABLE macro.
>>> read(2) might be interrupted by signal.
>>>
>>>
>>> > + if (n < 0) {
>>> > + if (errno == EWOULDBLOCK) {
>>>
>>> According to man page, read(2) sets EWOULDBLOCK or EAGAIN.
>>> So you should check both errno.
>>>
>>>
>>> > + for (int i = str_count; i < expected_str_count; i++) {
>>> > + //timeout, fill reminded arguments with \0;
>>> > + buf[off+i] = '\0';
>>> > + str_count++;
>>> > + }
>>>
>>> You set zero to buf[] in above.
>>> So you can remove this loop, and set str_count to expected_str_count
>>> without manipulating buf[].
>>>
>>> In addition, I prefer to add log_debug() at this
>>> to record NULL arguments are added.
>>>
>>>
>>> > + break;;
>>> > + }
>>> > + }
>>> > assert(n <= left, "buffer was too small, impossible!");
>>> > buf[max_len - 1] = '\0';
>>> > if (n == -1) {
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> > Thanks.
>>> > Lin
>>> >
>>> > ________________________________________
>>> > From: Yasumasa Suenaga <yasuenag at gmail.com
>>> <mailto:yasuenag at gmail.com>>
>>> > Sent: Wednesday, February 27, 2019 15:15
>>> > To: David Holmes; 臧琳
>>> > Cc: serviceability-dev at openjdk.java.net
>>> <mailto:serviceability-dev at openjdk.java.net>
>>> > Subject: Re: Protocol version of Attach API
>>> >
>>> > On 2019/02/27 15:59, David Holmes wrote:
>>> >> On 27/02/2019 4:10 pm, Yasumasa Suenaga wrote:
>>> >>> Hi,
>>> >>>
>>> >>> Buffer size for receiving packets from client is determined
>>> at [1].
>>> >>
>>> >> Maximum buffer size, yes.
>>> >>
>>> >>> It defines length of command name and of argument.
>>> >>> It is passed via Unix domain, so we fill '\0' to remaining
>>> bytes and
>>> >>> might be able to assume all arguments are passed with empty
>>> string.
>>> >>
>>> >> Not sure what you mean.
>>> >>
>>> >> // The buffer is expected to be formatted as follows:
>>> >> // <ver>0<cmd>0<arg>0<arg>0<arg>0
>>> >>
>>> >> so we can expect to read at least two things - the ver and cmd.
>>> If we encounter 00 we can infer we reached the end. But we may not
>>> have read the full data into the buffer, so can't tell if another
>>> call to read() is needed yet - you only know after you've read the
>>> 00.
>>> >>
>>> >>> BTW length of arguments is defined to 1024 in [2].
>>> >>> jcmd and jmap might pas file path - it might be JVM_MAXPATHLEN
>>> (4097 bytes).
>>> >>> Buffer size which is used in AttachListener seems not to be
>>> enough.
>>> >>
>>> >> One has to assume/hope that the code sending the data is working
>>> to the same protocol rules as the receiver! Otherwise this is just
>>> completely broken.
>>> >
>>> > On Linux, client (VirtualMachineImpl) seems not to check length
>>> of arguments:
>>> >
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java#l168
>>>
>>> >
>>> > In case of jcmd, all options are passed to arg #1. It seems not
>>> to check the length:
>>> >
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java#l111
>>>
>>> >
>>> >
>>> > I guess other tools (jstack, jmap, etc) which connect to
>>> AttachListener are same.
>>> > So we can fix both Attach API and AttachListener (it will be big
>>> change!),
>>> > but I concern we can keep protocol version...
>>> >
>>> >
>>> > Thanks,
>>> >
>>> > Yasumasa
>>> >
>>> >
>>> >> David
>>> >> -----
>>> >>
>>> >>> We might have to change more.
>>> >>>
>>> >>>
>>> >>> Thanks,
>>> >>>
>>> >>> Yasumasa
>>> >>>
>>> >>>
>>> >>> [1]
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/os/linux/attachListener_linux.cpp#l254
>>>
>>> >>> [2]
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/share/services/attachListener.hpp#l106
>>>
>>> >>>
>>> >>>
>>> >>> On 2019/02/27 15:00, 臧琳 wrote:
>>> >>>> Another solution I can figure out is try to add timeout for
>>> socket read. I will also investigate whether is works.
>>> >>>>
>>> >>>> Cheers,
>>> >>>> Lin
>>> >>>>
>>> >>>>
>>> >>>>> -----Original Message-----
>>> >>>>> From: 臧琳
>>> >>>>> Sent: Wednesday, February 27, 2019 1:52 PM
>>> >>>>> To: 'David Holmes' <david.holmes at oracle.com
>>> <mailto:david.holmes at oracle.com>>; Yasumasa Suenaga
>>> >>>>> <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>
>>> >>>>> Cc: serviceability-dev at openjdk.java.net
>>> <mailto:serviceability-dev at openjdk.java.net>
>>> >>>>> Subject: RE: Protocol version of Attach API
>>> >>>>>
>>> >>>>> Dear David, Yasumasa,
>>> >>>>> I think it is hard to know how long the buffer is
>>> passed from socket
>>> >>>>> without changing the info of the message from the receiver
>>> side.
>>> >>>>> So how about when str_count reach to 3, we test it
>>> with non_blocking
>>> >>>>> read?
>>> >>>>>
>>> >>>>>
>>> >>>>> Cheers,
>>> >>>>> Lin
>>> >>>>>> -----Original Message-----
>>> >>>>>> From: David Holmes <david.holmes at oracle.com
>>> <mailto:david.holmes at oracle.com>>
>>> >>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>> >>>>>> To: Yasumasa Suenaga <yasuenag at gmail.com
>>> <mailto:yasuenag at gmail.com>>; 臧琳 <zanglin5 at jd.com
>>> <mailto:zanglin5 at jd.com>>
>>> >>>>>> Cc: serviceability-dev at openjdk.java.net
>>> <mailto:serviceability-dev at openjdk.java.net>
>>> >>>>>> Subject: Re: Protocol version of Attach API
>>> >>>>>>
>>> >>>>>> Hi Yasumasa,
>>> >>>>>>
>>> >>>>>> On 27/02/2019 1:05 pm, Yasumasa Suenaga wrote:
>>> >>>>>>> Hi Lin,
>>> >>>>>>>
>>> >>>>>>> My proposal is a just idea, so you need to tweak it.
>>> >>>>>>>
>>> >>>>>>> AttachListener receives raw command.
>>> >>>>>>> For example, jcmd is `jcmd\0<arg strings>`. Please see
>>> >>>>>>> HotSpotVirtualMachine and extended classes.
>>> >>>>>>>
>>> >>>>>>> In case of jcmd, I guess AttachListener will receive message
>>> >>>>>>> `<version>\0jcmd\0<args>\0\0\0` (I did not check it well).
>>> >>>>>>> So I guess we can add '\0' when `str_count` does not reach
>>> to maximum.
>>> >>>>>>
>>> >>>>>> I don't see how this approach can work. You have to know how
>>> many
>>> >>>>>> arguments are coming in the "packet", but that information
>>> is not
>>> >>>>>> available in the current Linux implementation.Without it you
>>> can't
>>> >>>>>> know when to stop calling read().
>>> >>>>>>
>>> >>>>>> The protocol would need to be changed to send the "packet"
>>> size, but
>>> >>>>>> that's not compatible with older JDKs.
>>> >>>>>>
>>> >>>>>> Otherwise I think we have no choice but to use a
>>> non-blocking read ...
>>> >>>>>> though I'm still unsure if you can know for certain that
>>> you've
>>> >>>>>> reached the end of the "packet" ...
>>> >>>>>>
>>> >>>>>> Thanks,
>>> >>>>>> David
>>> >>>>>>
>>> >>>>>>>
>>> >>>>>>> Thanks,
>>> >>>>>>>
>>> >>>>>>> Yasumasa
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>>> On 2019/02/27 11:50, zanglin5 at jd.com
>>> <mailto:zanglin5 at jd.com> wrote:
>>> >>>>>>>> Dear Yasumasa,
>>> >>>>>>>> The fix you mentioned seems not work as expected.
>>> >>>>>>>> I have done an experiment that use jdk1.8's "jcmd
>>> <pid> help" to
>>> >>>>>>>> attach to latest jdk.
>>> >>>>>>>> it seem the whole "jcmd <pid> help" buffer is not
>>> >>>>>>>> read completely at one time. in my case it is "jcmd",
>>> "<pid>",
>>> >>>>>>>> "help", and then block at while().
>>> >>>>>>>> After applied your change, it seems only the "jcmd" is
>>> >>>>>>>> processed, the "<pid>", "help" is replaced by '\0'.
>>> >>>>>>>>
>>> >>>>>>>> BRs,
>>> >>>>>>>> Lin
>>> >>>>>>>>
>>>
More information about the serviceability-dev
mailing list