On Fri, 30 Aug 2024 16:59:13 GMT, Leonid Mesnik <lmes...@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
