On Wed, 9 Apr 2025 23:12:00 GMT, Brent Christian <bchri...@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
