RFR: 8347779: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with Unable to deduce type of thread from address [v2]

[Chris Plummer]

On Wed, 22 Jan 2025 08:49:52 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

>> test/jdk/sun/tools/jhsdb/JShellHeapDumpTest.java line 104:
>> 
>>> 102:         if (!res) { // try once more
>>> 103:             launch(expectedMessage, Arrays.asList(toolArgs), false);
>>> 104:         }
>> 
>> `launch()` should take an allowRetry argument and only retry if true. This test is operated in two different modes. One is with an active target process and one is with a target process that should be stabilized and inactive. The latter is not allowed to retry since it should never fail. Also, it would help to add a comment as to why the retry is being done. See the comment in `printStackTrace()`.
>> 
>> I think a better approach would be for launch() to just return if a retry should be done. testHeapDump() should check the result, and just immediately return true if launch() returns true.
>
>> launch() should take an allowRetry argument and only retry if true.
> 
> Are you talking about `public static void launch(String expectedMessage, String... toolArgs)`   ?

Yes, but my suggestion on how to fix this might not be the best one. It's a bit hard to get the naming and usage of flags and logic right here. allowRetry, vs retry, vs doSleep. They all relate. Bottom line is that we should only ever attempt a retry if doSleep is false. Right now that flag is checked in printStackTraces(), and determines if true should be returned to allow a retry, but only if we aren't already doing a retry. Something similar needs to be done with launch() when it is deciding to return true or false after a failure. Right now it is only checking the allowRetry flag, but it should also check the doSleep flag.

I think in testHeapDump(), the calling of `launch(String expectedMessage, String... toolArgs)` should be done in a way similar to calling `printStackTraces()`. Pass in allowRetry and assign the result to the retry boolean. If retry is true, then return it right away (back to main() so it can retry). In `launch(String expectedMessage, String... toolArgs)`, only call `launch(String expectedMessage, List<String> toolArgs, boolean allowRetry)` once, passing in allowRetry flag. In `launch(String expectedMessage, List<String> toolArgs, boolean allowRetry)`, only return true to allow a retry if allowRetry is true and doSleep is false.

Anyway, it's kind of messy and there may be another way. The key issue with your current version is that it always allows for a retry, but it should only allow a retry if doSleep is false.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23213#discussion_r1925979789