On Fri, 30 Aug 2024 00:07:50 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> The fix improves Attch API protocol and implements updated protocol on > windows; shared code is ready to implement updated protocol support on other > platforms. > More detailed explanations on the 1st comment. > > Testing: tier1,tier2,tier3,tier4,hs-tier5-svc > manually tested backward compatibility (old tools can attach to current > VMs, current tools can attach to older VMs) on Windows with jdk21u and jdk8u. This is nice fix, thank you for doing this! I still need one more pass on the Windows `AttachListener` part and hope to complete it early next week. src/hotspot/share/services/attachListener.cpp line 649: > 647: > 648: return true; > 649: } Nit: This function is too big. I'd suggest to split it to make more readable. For example, the lines 596-648 can be moved to new function which is called by the `AttachOperation::read_request()'. src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java line 46: > 44: > 45: private volatile long hProcess; // handle to the process > 46: private int ver = VERSION_1; // updated by detectVersion on > attach Nit: The comment is not fully accurate as the result returned by the `detectVersion()` is stored in this private field by the `VirtualMachineImpl` constructor. ------------- PR Review: https://git.openjdk.org/jdk/pull/20782#pullrequestreview-2363889264 PR Review Comment: https://git.openjdk.org/jdk/pull/20782#discussion_r1797683041 PR Review Comment: https://git.openjdk.org/jdk/pull/20782#discussion_r1797678822
