RFR: 8305186: Reference.waitForReferenceProcessing should be more accessible to tests [v3]

[Brent Christian]

On Thu, 10 Apr 2025 05:51:04 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Brent Christian has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   reflection improvements
>
> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
> 
>> 568:    * This method should usually be called after a call to WhiteBox.fullGC().
>> 569:    */
>> 570:   public static void waitForReferenceProcessing() {
> 
> Reference::waitForReferenceProcessing returns boolean.  This should too.

Agreed.

> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
> 
>> 568:    * This method should usually be called after a call to WhiteBox.fullGC().
>> 569:    */
>> 570:   public static void waitForReferenceProcessing() {
> 
> Reference::waitForReferenceProcessing throws InterruptedException.  Probably this should be similarly
> declared.

I disagree. IMO, for a test library, it's an unnecessary burden to make callers catch a checked exception.

> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
> 
>> 568:    * This method should usually be called after a call to WhiteBox.fullGC().
>> 569:    */
>> 570:   public static void waitForReferenceProcessing() {
> 
> WhiteBox functions are nearly all ordinary member functions, not static.

I saw that, but `Reference.waitForReferenceProcessing()` is itself static, and doesn't need any context from `WhiteBox`, so I made it static.
But if `waitForReferenceProcessing()` is only expected to be called following `fullGC()`, a WB instance will be needed anyway.

> test/lib/jdk/test/whitebox/WhiteBox.java line 574:
> 
>> 572:       Method wfrp = Reference.class.getDeclaredMethod("waitForReferenceProcessing");
>> 573:       wfrp.setAccessible(true);
>> 574:       wfrp.invoke(null, new Object[0]);
> 
> Don't need an empty argument vector. Sufficient is `wfrp.invoke(null);`.  I found the documentation
> a bit confusing, as it says "the supplied args array may be ... or null."

Thanks. (I thought I tried that, but I guess not...)

> test/lib/jdk/test/whitebox/WhiteBox.java line 577:
> 
>> 575:     } catch (IllegalAccessException iae) {
>> 576:       throw new RuntimeException("Need to add @modules java.base/java.lang.ref:open?",
>> 577:               iae);
> 
> This isn't a useful message, as we won't get here in this case.  Instead of `invoke` failing with this exception,
> the earlier `setAccessible` will have failed with `InaccessibleObjectException`.

Thanks. The new version will include the message in the case that the @modules tag seems to be missing.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038425087
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038428891
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038432164
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038425699
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038424768