On Thu, 6 Feb 2025 20:23:32 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> The fix implements streaming output support for attach protocol.
>> See JBS issue for evaluation, summary of the changes in the 1st comment.
>> Testing: tier1..4,hs-tier5-svc
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback - test
Hi,
Thanks for waiting.
I understand, I think, more or less what this does on the VM side now. I think
that we can simplify the code for the cases when we set an error and don't
write any error message.
src/hotspot/os/posix/attachListener_posix.cpp line 150:
> 148: }
> 149:
> 150: void complete(jint res, bufferedStream* st) override;
Style: Put brackets together `{}`
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`.
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`?
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
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`?
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`.
src/hotspot/share/services/attachListener.cpp line 137:
> 135: if (!_error) {
> 136: _error = !_reply_writer->write_fully(str, (int)len);
> 137: }
What happens if we're in `is_streaming()` but the result isn't written? That
breaks the protocol. At least add an assert that `_result_written` is set to
true.
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23405#pullrequestreview-2660667054
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981124805
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981125292
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981131134
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981131731
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981377287
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981373628
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981629725
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1981584499
