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

[Kim Barrett]

On Wed, 9 Apr 2025 23:12:00 GMT, Brent Christian <bchristi at openjdk.org> wrote:

>> Certain specific types of tests involving GC and reference processing need to account for the delay between a GC completing (during which the GC clears a Reference), and the Reference being added to its associated queue. At present, ad hoc mechanisms (with delays/timeout) are used, but can lead to intermittent test failures ([JDK-8298783](https://bugs.openjdk.org/browse/JDK-8298783)  is a recent example).
>> 
>> A better mechanism already exists in the private `Reference.waitForReferenceProcessing()` method. This PR makes `waitForReferenceProcessing()` available to tests via the `WhiteBox` and `ForceGC` test libraries.
>
> Brent Christian has updated the pull request incrementally with one additional commit since the last revision:
> 
>   reflection improvements

I made enough detailed suggestions that I decided to offer up what I think the
end result might look like.  I've done some minimal testing of this.

  private Method waitForReferenceProcessingMethod = null;

  private Method getWaitForReferenceProcessingMethod() {
    Method wfrp = waitForReferenceProcessingMethod;
    if (wfrp == null) {
      try {
        wfrp = Reference.class.getDeclaredMethod("waitForReferenceProcessing");
      } catch (NoSuchMethodException e) {
        throw new RuntimeException("Missing Reference::waitForReferenceProcessing");
      }
      try {
        wfrp.setAccessible(true);
      } catch (InaccessibleObjectException e) {
        throw new RuntimeException("Need to add @modules java.base/java.lang.ref:open to test?", e);
      }
      assert wfrp.getReturnType() == Boolean.class;
      assert wfrp.getParameterCount() == 0;
      Class<?>[] ev = wfrp.getExceptionTypes();
      assert ev.length == 1;
      assert ev[0] == InterruptedException.class;
      waitForReferenceProcessingMethod = wfrp;
    }
    return wfrp;
  }

  public boolean waitForReferenceProcessing() throws InterruptedException {
    Method wfrp = getWaitForReferenceProcessingMethod();
    try {
      Boolean result = (Boolean) wfrp.invoke(null);
      return result.booleanValue();
    } catch (IllegalAccessException e) {
      throw new RuntimeException(e);
    } catch (InvocationTargetException e) {
      Throwable cause = e.getCause();
      if (cause instanceof InterruptedException) {
        throw (InterruptedException) cause;
      } else if (cause instanceof RuntimeException) {
        throw (RuntimeException) cause;
      } else {
        throw (Error) cause;
      }
    }
  }

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.

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.

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.

test/lib/jdk/test/whitebox/WhiteBox.java line 572:

> 570:   public static void waitForReferenceProcessing() {
> 571:     try {
> 572:       Method wfrp = Reference.class.getDeclaredMethod("waitForReferenceProcessing");

Why was the caching of the method removed in the latest commit?

It seems like it might be cleaner to split this into a helper that gets the Method object, with a catch clause
for NoSuchMethodException.  Such a helper would also be a place to verify/assert various properties of
the found method, such as empty parameter list, return type is boolean, one declared exception type
(InterruptedException), all of which can then be assumed by the invocation.

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."

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`.

test/lib/jdk/test/whitebox/WhiteBox.java line 578:

> 576:       throw new RuntimeException("Need to add @modules java.base/java.lang.ref:open?",
> 577:               iae);
> 578:     } catch (NoSuchMethodException | InvocationTargetException e) {

I think for InvocationTargetException the appropriate thing to do is to rethrow the cause, which will
require dispatching on its dynamic type in order to cast to an appropriate static type.  The only checked
exception is InterruptedException.  But it could also be RuntimeException or Error.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24527#pullrequestreview-2755384786
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036541359
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036542867
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036554417
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036536961
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036549322
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036707567
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2036573588