RFR: 8319055: JCMD should not buffer the whole output of commands [v4]

[Alex Menkov]

On Thu, 6 Mar 2025 17:27:56 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   feedback
>
> src/hotspot/share/services/attachListener.cpp line 316:
> 
>> 314: 
>> 315: static jint data_dump(AttachOperation* op, attachStream* out) {
>> 316:   out->set_result(JNI_OK); // allow streaming output
> 
> I'm wondering, do you think it's useful to add a helper `start_streaming_if_available` (or whatever) that just calls `set_result(JNI_OK)`? That helper could have a small snippet of documentation to explain what's going on. That would probably be neater than a comment.

I don't think helper method is needed (and those comments too :)
I think the better approach would be "operation handler should call set_result as soon as it knows the result code"

> src/hotspot/share/services/attachListener.cpp line 373:
> 
>> 371:   // The commands report error if the agent failed to load, so we need to disable streaming output.
>> 372:   const char jmx_prefix[] = "ManagementAgent.";
>> 373:   if (strncmp(op->arg(0), jmx_prefix, strlen(jmx_prefix)) == 0) {
> 
> `const char* jmx_prefix = "ManagementAgent.", not array.

Fixed.

> src/hotspot/share/services/attachListener.cpp line 396:
> 
>> 394:   if (HAS_PENDING_EXCEPTION) {
>> 395:     // We can get an exception during command execution.
>> 396:     // In the case _attach_stream->set_result() is already called and operation result code is send
> 
> "is senT to the client"

Fixed

> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line 379:
> 
>> 377: 
>> 378:             // Reply is "<ver> option1,option2...".
>> 379:             int delimPos = message.indexOf(' ');
> 
> We can do this in an early-break sort of way. That'd reduce the indentation and let's us not to have to check `delimPos` again.
> 
> 
> int delimPos = message.indexOf(' ');
> int supportedVersion = Integer.parseUnsignedInt(delimPos < 0 ? message : message.substring(0, delimPos));
> if (delimPos < 0 || supportedVersion != VERSION_2) {
>     return; // No options to parse, or doesn't support options anyway
> }
> /* Insert the parsing stuff here */

To me current implementation looks clearer. And I think it will be simpler to extend the logic (if we ever need to extend v2 or introduce v3)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984330438
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984331040
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984332637
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984343797