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

[Alex Menkov]

On Wed, 5 Mar 2025 10:29:21 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 - test
>
> src/hotspot/os/posix/attachListener_posix.cpp line 150:
> 
>> 148:   }
>> 149: 
>> 150:   void complete(jint res, bufferedStream* st) override;
> 
> Style: Put brackets together `{}`

Fixed

> src/hotspot/os/posix/attachListener_posix.cpp line 152:
> 
>> 150:   void complete(jint res, bufferedStream* st) override;
>> 151: 
>> 152:   virtual ReplyWriter* get_reply_writer() override {
> 
> Style: No need for both `virtual` and `override`.

Fixed.

> src/hotspot/os/windows/attachListener_windows.cpp line 128:
> 
>> 126:     if (!fSuccess) {
>> 127:       log_error(attach)("pipe write error (%d)", GetLastError());
>> 128:       return -1;
> 
> Style wish: Could you rename `fSuccess` to something like  `write_success`?

This is general style in the file (I believe it came from MSDN examples), so I prefer to leave it as is for now

> src/hotspot/os/windows/attachListener_windows.cpp line 159:
> 
>> 157:   void complete(jint result, bufferedStream* result_stream) override;
>> 158: 
>> 159:   virtual ReplyWriter* get_reply_writer() override {
> 
> Style: virtual && override unnecessary

Fixed.

> src/hotspot/share/services/attachListener.cpp line 63:
> 
>> 61: class attachStream : public bufferedStream {
>> 62:   AttachOperation::ReplyWriter* _reply_writer;
>> 63:   bool _allow_streaming;
> 
> Is this `const`?

Right. Fixed.

> src/hotspot/share/services/attachListener.cpp line 66:
> 
>> 64:   jint _result;
>> 65:   bool _result_set;
>> 66:   bool _result_written;
> 
> It seems like `_result_written` implies `_result_set`? You can do this:
> 
> ```c++
> enum class ResultState { Unset; Set; Written; };
> ResultState _result_state;
> jint _result;
> 
> 
> And then have the `ResultState` transition from `Unset` to `Set` to `Written`.

Updated

> src/hotspot/share/services/attachListener.cpp line 213:
> 
>> 211: }
>> 212: 
>> 213: static void get_properties(AttachOperation* op, attachStream* out, Symbol* serializePropertiesMethod) {
> 
> In this function you have to remember to set the result, otherwise you'll accidentally drop the result. If you return `jint`, you're forced to remember to write a  `return`.
> 
> I'd say that you should revert the changes for this function, and instead have the callers of this method set the result code.
> 
> In fact, revert this kind of change for all functions in `AttachOperationFunctionInfo funcs[]` and have all send the return code. That simplifies the pattern significantly, as you only need to set the result early if you want to be streaming your output. That also minimizes the changeset, win-win.

Updated as suggested. But github considers diff for the file as "large" :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982500667
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982501693
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982528121
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982502593
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982504691
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982517135
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982514245