Hi Dan,
Thanks you! Not sure how I realized that I needed to set +e for the grep
command but it did not occur to me to do the same thing for the jcmd
invocation.
Here is the new webrev:
http://cr.openjdk.java.net/~dbuck/7154822/webrev.02/
Please let me know what you think.
I tested new version of the script locally (solaris/SPARC). Rerunning on
JPRT just to make sure all platforms are OK. (I'm a sucker for testing.)
Cheers,
-Buck
On 3/30/2012 12:36 PM, Daniel D. Daugherty wrote:
On 3/29/12 9:04 PM, David Buck wrote:
Hi!
I would like to submit a new version of the fix for review:
http://cr.openjdk.java.net/~dbuck/7154822/webrev.01/
src/share/classes/sun/tools/jcmd/JCmd.java
No comments.
test/sun/tools/jcmd/dcmd-big-script.txt
No comments.
test/sun/tools/jcmd/jcmd-big-script.sh
Since your test is using 'set +e' to avoid a premature exit due
to 'grep' exiting with a non-zero exit code, you also need to
handle a non-zero exit code from JCMD. I'm guessing that one
of the common/... script is calling the original 'set -e'.
Please consider:
set +e
${JCMD} -J-XX:+UsePerfData $appJavaPid -f ${TESTSRC}/dcmd-big-script.txt
> jcmd.out 2>&1
status="$?"
set -e
and:
set +e #if the test passes, grep will "fail" with an exit code of 1
grep Exception jcmd.out > /dev/null 2>&1
status="4?"
set -e
if [ "$status" = 0 ]; then
Sorry I missed this the first time.
Dan
Changes:
JCmd.java :
1. fixed copyright statement (s/2012/2011, 2012/)
2. changed regular expression used to split up script into lines from
"\\r?\\n" to "\\n"
dcmd-big-script.txt :
3. removed extraneous blank line at end of dcmd-big-script.txt
jcmd-big-script.sh :
4. improved output message when jcmd returns a non-zero exit code so
that it prints out the exit code jcmd returned.
5. redirected stdout/stderr from grep command
Change 2 warrants further explanation. I do not want to use "$" as the
regex to split up the input because it will register an additional
line between the final command's "\n" and the end of the string. So I
would need to add code to remove these additional empty "lines". I see
no need to further complicate the code.
If we look at how the command string is generated
(src/share/classes/sun/tools/jcmd/Arguments.java), we see that we have
already read in the file line by line and then use a StringBuilder to
paste it back together (with an added "\n" at the end of each line).
So it turns out that the regex I originally chose, "\\r?\\n", was
unnecessarily complicated as the string we are reading in is not the
raw file, but a string we built ourselves with our own chosen line
separator, "\n". Since we know for sure that there is no CRs (\r)
dividing our lines, we can (and should) just use "\\n". This way we
mirror the way we put the sting together in the first place. Sorry
about not catching that sooner.
Hopefully the fix should now be ready to push. Please let me know what
you think. Thank you all for the feedback.
Cheers,
-Buck
On 3/30/2012 3:20 AM, Staffan Larsen wrote:
On 29 mar 2012, at 17:11, David Buck wrote:
Hi!
If the limit is the same for all platforms, then the fix
could be improved to check that each line is smaller than the
limit before to send it to the target JVM, and properly
report an error if it isn't. Right now, each platform
throws an IOException with a platform dependent message.
Instead, detecting lines exceeding the limit and printing
a clear message like:
"line 84: Line too long"
would improve the user experience.
That should be doable. Are we OK with hard-coding the current VM
limit
into the client side code?
I'm not fond of the idea that the client side "knows" the VM limit.
Yes, I recognize that would improve the user experience, but it
makes the code more fragile. I don't think there is a client-side
query for determining the buffer length. Perhaps you should query
Alan Bateman for advice since the attach-on-demand stuff was his
creating way back when... He might have some advice...
I talked with Dan over IM about this, and feel that it is not worth
pursuing. There is no obvious interface for a client to dynamically
query the limit. And when an exception is thrown, there is no
obvious way for the client to distinguish between having sent a
string that was too large and some other IO error. Hard coding this
value into the client is just asking for trouble. Ideally, jcmd
should be inter-operable with multiple versions of the JVM.
It may be possible to use the error messages sent from the VM to the
client to do this. See the ATTACH_ERROR_* constants in
attachListener_solaris.cpp for example. Similar things exists on
linux, but apparently not for windows. Anyway, that would be outside
the scope of this fix.
Regards,
/Staffan
