RFR: 8356811: Some nsk/jdi tests can fetch ThreadReference from static field in the debuggee: part 4

[Chris Plummer]

On Tue, 13 May 2025 22:37:40 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> test/hotspot/jtreg/vmTestbase/nsk/jdi/StackFrame/setValue/setvalue005/setvalue005.java line 165:
>> 
>>> 163:             debuggee.threadByFieldName(rType, "mainThread", DEBUGGEE_THRDNAME);
>>> 164:         if (thrRef == null) {
>>> 165:             log.complain("TEST FAILURE: method Debugee.threadByName() returned null for debuggee thread "
>> 
>> Need to update error message
>> BTW `debuggee.classByName` also can throw exceptions, need to catch and call `quitDebuggee()` (this applicable to other changes in the fix when `debuggee.classByName` is called outside of try block)
>
> This is a common pattern in my changes. I'l need to add something like the following catch:
> 
> 
>         } catch (Exception e) {
>             e.printStackTrace();
>             log.complain("TEST FAILURE: caught unexpected exception: " + e);
>             tot_res = Consts.TEST_FAILED;
>         }
> 
> 
> It would have been a lot cleaner if these tests were all written to just let the exception happen and not worry about printing messages about what it was doing when the exception was thrown. The exception stack trace provides all the info you need.
> 
> The error handling in these tests is frustrating to work with. It's inconsistent and incomplete. Many catch exceptions, print a failure message (and no exception backtrace), and them resume at the next step, only to hang as a result.
> 
> I was tempted to get rid of a lot of the failure handling when thrRef == null and just use threadByFieldNameOrThrow() instead of threadByFieldName(), but also wanted to limit the scope of the changes. I did observe that in general this works well. While making the changes for this PR I dealt with a lot of failures due to calling threadByFieldNameOrThrow() with bad args, and the test always failed quickly and gracefully. But in the end I changed to threadByFieldName() to keep things a bit more consistent.

Actually debuggee.classByName() returns null if the class is not found, and callers typically do not check for this, which results in an NPE later on. debuggee.classByName() can also throw an exception if it discovers something like multiple matches for the class name.

I'm not so sure the added error handling is of any real benefit here, and just adds to the already overdone error handling. Exceptions seem to properly result in a quick exit, and in fact it is the error handling that can result in not exiting quickly due to trying to sync with the debuggee. I'd like to request not having to add additional checks when calling debuggee.classByName()

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25190#discussion_r2089514774