RFR: 8039173 Propagate errors from Diagnostic Commands as exceptions in the attach framework
Ivan Gerasimov
ivan.gerasimov at oracle.com
Fri Apr 11 07:32:26 UTC 2014
Hi Staffan!
Isn't the second read in line# 268 erroneous?
267 while ((n = sis.read(b)) != -1) {
268 n = sis.read(b);
Sincerely yours,
Ivan
On 11.04.2014 11:16, Staffan Larsen wrote:
> Here is a new version where the 200 char cap is removed:
>
> http://cr.openjdk.java.net/~sla/8039173/webrev.02/
>
> Thanks,
> /Staffan
>
> On 4 apr 2014, at 16:27, Staffan Larsen <staffan.larsen at oracle.com> wrote:
>
>> Thanks Ivan.
>>
>> I’ve been thinking a bit more about this and discussing with people here. I’m not sure anymore that it is a good idea to cap the error message. My original thought was that it made sense to not overflow the client with tons of output if something went wrong in the target. On the other hand, there is nothing that says that the error message will be in the first 200 characters. It is perfectly possible for an operation to successfully run for a while and only at the end run into a problem. In that case the output will have the exception at the end, following any amount of output. Capping the message in that case would hide the error.
>>
>> (My second thought was: let’s only take the last 200 chars, but there is no guarantee that that will contain the complete error message either).
>>
>> So the "keep it simple” principle here would be to remove the cap (which also makes the code in getErrorMessage() simpler).
>>
>> If someone has a better solution for propagating error messages from the operations, I’m all ears.
>>
>> Thanks,
>> /Staffan
>>
>>
>>
>> On 4 apr 2014, at 14:41, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>
>>> An alternative, more compact variant might be
>>>
>>> ------------------------
>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>> int n, off = 0, len = maxlen + 1;
>>> byte b[] = new byte[len];
>>> while ((n = sis.read(b, off, len - off)) > 0)
>>> off += n;
>>> return (off == 0) ? null
>>> : (off < len)
>>> ? new String(b, 0, off, "UTF-8")
>>> : new String(b, 0, maxlen, "UTF-8") + " ...";
>>> }
>>> ------------------------
>>>
>>> Not a big deal, of course.
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>>
>>>
>>> On 04.04.2014 16:24, Ivan Gerasimov wrote:
>>>> Now you reintroduced the smallish issue, when the message is exactly 200 characters (or whatever maxlen is).
>>>> The dots will be appended to the message, even though it's not necessary.
>>>>
>>>> I think the only reliable way to deal with it is to try to read one extra character from sis.
>>>>
>>>> Something like this should do:
>>>> ------------------------
>>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>>> byte b[] = new byte[maxlen + 1];
>>>> int n, off = 0, len = b.length;
>>>> do {
>>>> n = sis.read(b, off, len);
>>>> if (n == -1) {
>>>> break;
>>>> }
>>>> off += n;
>>>> len -= n;
>>>> } while (off < maxlen);
>>>>
>>>> String message = null;
>>>> if (off > 0) {
>>>> message = (off > maxlen)
>>>> ? new String(b, 0, maxlen, "UTF-8") + " ..."
>>>> : new String(b, 0, off, "UTF-8");
>>>> }
>>>> return message;
>>>> }
>>>> ------------------------
>>>>
>>>> Sincerely yours,
>>>> Ivan
>>>>
>>>> On 04.04.2014 16:08, Staffan Larsen wrote:
>>>>> I’m afraid you are right! Doh. Need to add more testing...
>>>>>
>>>>> How about this change:
>>>>>
>>>>> --- a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>> +++ b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>> @@ -267,9 +267,11 @@
>>>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>>>> byte b[] = new byte[maxlen];
>>>>> int n, off = 0, len = maxlen;
>>>>> + boolean complete = false;
>>>>> do {
>>>>> n = sis.read(b, off, len);
>>>>> if (n == -1) {
>>>>> + complete = true;
>>>>> break;
>>>>> }
>>>>> off += n;
>>>>> @@ -280,7 +282,7 @@
>>>>> if (off > 0) {
>>>>> message = new String(b, 0, off, "UTF-8");
>>>>> }
>>>>> - if (off > b.length && message != null) {
>>>>> + if (!complete && message != null) {
>>>>> message += " ...";
>>>>> }
>>>>> return message;
>>>>>
>>>>>
>>>>> On 4 apr 2014, at 13:55, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>>>>
>>>>>> Thank you Staffan for fixing them!
>>>>>>
>>>>>> But I'm afraid that now the function will never add ellipsis to the message, even if it gets truncated.
>>>>>>
>>>>>> Sincerely yours,
>>>>>> Ivan
>>>>>>
>>>>>> On 04.04.2014 15:47, Staffan Larsen wrote:
>>>>>>> Thanks for finding these bugs, Ivan!
>>>>>>>
>>>>>>> I have updated the webrev at: http://cr.openjdk.java.net/~sla/8039173/webrev.01/, and I have also included the diff below.
>>>>>>>
>>>>>>> The updated webrev also has some changes in the javadoc for VirtualMachine to clarify that some methods can now throw AttachOperationFailedException.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> /Staffan
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>> --- a/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>> +++ b/src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
>>>>>>> @@ -266,18 +266,21 @@
>>>>>>> */
>>>>>>> String getErrorMessage(InputStream sis, int maxlen) throws IOException {
>>>>>>> byte b[] = new byte[maxlen];
>>>>>>> - int n, off = 0, len = b.length;
>>>>>>> + int n, off = 0, len = maxlen;
>>>>>>> do {
>>>>>>> n = sis.read(b, off, len);
>>>>>>> + if (n == -1) {
>>>>>>> + break;
>>>>>>> + }
>>>>>>> off += n;
>>>>>>> len -= n;
>>>>>>> - } while (n >= 0 && off < b.length);
>>>>>>> + } while (off < maxlen);
>>>>>>>
>>>>>>> String message = null;
>>>>>>> if (off > 0) {
>>>>>>> message = new String(b, 0, off, "UTF-8");
>>>>>>> }
>>>>>>> - if (off == b.length && message != null) {
>>>>>>> + if (off > b.length && message != null) {
>>>>>>> message += " ...";
>>>>>>> }
>>>>>>> return message;
>>>>>>>
>>>>>>>
>>>>>>> On 4 apr 2014, at 11:18, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>>>>>>
>>>>>>>> Hi Staffan!
>>>>>>>>
>>>>>>>> I think there is a couple of minor bugs in getErrorMessage(InputStream sis, int maxlen).
>>>>>>>>
>>>>>>>> 1) If maxlen is exactly the size of the message to read, the function will add an ellipsis, even though the message isn't truncated,
>>>>>>>> 2) If maxlen is greater than needed, then sis.read(b, off, len) at the line #271 will eventually return -1, and it will cause the message to lose its last character.
>>>>>>>>
>>>>>>>> Sincerely yours,
>>>>>>>> Ivan
>>>>>>>>
>>>>>
>>>>
>>>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140411/4d4598be/attachment-0001.html>
More information about the serviceability-dev
mailing list