RFR: 8338708: Don't create/destroy debug agent cmdQueueLock for each connection.

[Chris Plummer]

On Fri, 30 Aug 2024 16:59:13 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> Only allocate the cmdQueueLock once rather than allocate each time there is a new connection (and destroy when the connection is terminated).
>> 
>> Currently each time there is a new debug agent connection established, the cmdQueueLock is allocated. It is then destroyed when the connection is dropped, only to be recreated on the next connection. I looked closely at the use of this lock, and what happens when the connection is dropped, and I can't see any reason to create and then dispose of cmdQueueLock for each connection. Of the 18 debug agent raw monitors, this is the only one that gets destroyed and recreated. The other 17 stick around for the next connection. I'd like to get rid of this reallocation in order to support static initialization of all the debug agent raw monitors on startup. This will be done as part of the ranked monitor support ([JDK-8328866](https://bugs.openjdk.org/browse/JDK-8328866)).
>> 
>> As part of the testing for this change, I put an assert in place when we try to create cmdQueueLock for the 2nd time. I ran all our debugger tests, and unfortunately none of them failed. This means we don't currently test attaching to the debug agent more than once in any of our tests, so I did two things to better test out this change. The first was to write a test that attaches numerous times in sequence, and the 2nd was to manually attach and reattach using jdb. At the same time I stepped through the debug agent code in gdb just to help better understand the setup and use of cmdQueueLock. I didn't see anything that gave me concern about making this change.
>> 
>> Tested by running all jdi, jdwp, and jdb tests on all supported platforms.
>
> test/jdk/com/sun/jdi/ReattachStressTest.java line 76:
> 
>> 74:             // Read the first character of output to make sure we've waited until the
>> 75:             // debuggee is ready. This will be the debug agent's "Listening..." message.
>> 76:             InputStream is = p.getInputStream();
> 
> The stream is not closed. Not sure it can cause problems but probably could once we reuse agent for tests.

The `InputStream` is already created when the `Process` is spawned. This API is simply returning it. It doesn't seem appropriate to close it in this case. It should be closed automatically when the Process is destroyed. I also see a lot of other uses of `p.getInputStream()` that don't explicitly close, including some which are obviously not closed:

`        p.getInputStream().read();`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20755#discussion_r1739377801