Invoke @⁠AutoClose methods via interfaces whenever possible

Prior to this commit, the @⁠AutoClose extension attempted to invoke a
"close" method via the most specific "close" method found in the type
hierarchy. However, this resulted in an InaccessibleObjectException if
the "close" method was implemented in a non-public type from a
different module (such as from the java.base module).

In order to avoid issues with the enforcement of strict encapsulation
within the JVM, this commit attempts to always invoke a "close" method
via the declaring interface.

For a "close" method in a type that implements AutoCloseable, we now
simply cast the object to AutoCloseable and invoke the "close()" method
directly, without using reflection.

For any other method, we use the new getInterfaceMethodIfPossible()
utility in ReflectionUtils to locate an interface method (if such a
method exists) and invoke that.

The latter was graciously donated by @jhoeller from the Spring
Framework.

Closes #3684

Author: sbrannen

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/AutoCloseExtension.java

@@ -80,12 +80,20 @@ private static void closeField(Field field, Object testInstance) throws Exceptio
 		}
 	}
 
-	private static void invokeCloseMethod(Field field, Object target, String methodName) {
+	private static void invokeCloseMethod(Field field, Object target, String methodName) throws Exception {
+		// Avoid reflection if we can directly invoke close() via AutoCloseable.
+		if (target instanceof AutoCloseable && "close".equals(methodName)) {
+			((AutoCloseable) target).close();
+			return;
+		}
+
 		Class<?> targetType = target.getClass();
 		Method closeMethod = ReflectionUtils.findMethod(targetType, methodName).orElseThrow(
 			() -> new ExtensionConfigurationException(
 				String.format("Cannot @AutoClose field %s because %s does not define method %s().",
 					getQualifiedName(field), targetType.getName(), methodName)));
+
+		closeMethod = ReflectionUtils.getInterfaceMethodIfPossible(closeMethod, targetType);
 		ReflectionUtils.invokeMethod(closeMethod, target);
 	}
 

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/AutoCloseTests.java

@@ -18,10 +18,13 @@
 import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure;
 import static org.junit.platform.testkit.engine.TestExecutionResultConditions.message;
 
+import java.io.InputStream;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 
@@ -115,6 +118,18 @@ void spyPermitsOnlyASingleAction() {
 		assertThat(recorder).containsExactly("AutoCloseTests.preconditions.close()");
 	}
 
+	/**
+	 * @see <a href="https://github.com/junit-team/junit5/issues/3684">#3684</a>
+	 */
+	@Test
+	void fieldsAreProperlyClosedViaInterfaceMethods() {
+		// If the test method succeeds, that means there was no issue invoking
+		// the @AutoClose fields. No need to assert anything else for this use case.
+		executeTestsForClass(CloseMethodMustBeInvokedViaInterfaceTestCase.class)//
+				.testEvents()//
+				.assertStatistics(stats -> stats.succeeded(1));
+	}
+
 	@Test
 	void fieldsAreProperlyClosedWithInstancePerMethodTestClass() {
 		Events tests = executeTestsForClass(InstancePerMethodTestCase.class).testEvents();
@@ -432,6 +447,15 @@ static class NoShutdownMethodTestCase implements TestInterface {
 		private final String field = "";
 	}
 
+	static class CloseMethodMustBeInvokedViaInterfaceTestCase implements TestInterface {
+
+		@AutoClose
+		final InputStream inputStream = InputStream.nullInputStream();
+
+		@AutoClose("shutdown")
+		final ExecutorService service = Executors.newSingleThreadExecutor();
+	}
+
 	@TestInstance(PER_METHOD)
 	static class InstancePerMethodTestCase {
 

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

@@ -11,6 +11,7 @@
 package org.junit.platform.commons.util;
 
 import static java.lang.String.format;
+import static java.util.Collections.synchronizedMap;
 import static java.util.stream.Collectors.toCollection;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
@@ -125,6 +126,13 @@ public enum HierarchyTraversalMode {
 	private static final ClasspathScanner classpathScanner = new ClasspathScanner(
 		ClassLoaderUtils::getDefaultClassLoader, ReflectionUtils::tryToLoadClass);
 
+	/**
+	 * Cache for equivalent methods on an interface implemented by the declaring class.
+	 * @since 1.11
+	 * @see #getInterfaceMethodIfPossible(Method, Class)
+	 */
+	private static final Map<Method, Method> interfaceMethodCache = synchronizedMap(new LruCache<>(255));
+
 	/**
 	 * Set of fully qualified class names for which no cycles have been detected
 	 * in inner class hierarchies.
@@ -1312,6 +1320,54 @@ public static Try<Method> tryToGetMethod(Class<?> clazz, String methodName, Clas
 		return Try.call(() -> clazz.getMethod(methodName, parameterTypes));
 	}
 
+	/**
+	 * Determine a corresponding interface method for the given method handle, if possible.
+	 * <p>This is particularly useful for arriving at a public exported type on the Java
+	 * Module System which can be reflectively invoked without an illegal access warning.
+	 * @param method the method to be invoked, potentially from an implementation class
+	 * @param targetClass the target class to check for declared interfaces
+	 * @return the corresponding interface method, or the original method if none found
+	 * @since 1.11
+	 */
+	@API(status = INTERNAL, since = "1.11")
+	public static Method getInterfaceMethodIfPossible(Method method, Class<?> targetClass) {
+		if (!isPublic(method) || method.getDeclaringClass().isInterface()) {
+			return method;
+		}
+		// Try cached version of method in its declaring class
+		Method result = interfaceMethodCache.computeIfAbsent(method,
+			m -> findInterfaceMethodIfPossible(m, m.getDeclaringClass(), Object.class));
+		if (result == method && targetClass != null) {
+			// No interface method found yet -> try given target class (possibly a subclass of the
+			// declaring class, late-binding a base class method to a subclass-declared interface:
+			// see e.g. HashMap.HashIterator.hasNext)
+			result = findInterfaceMethodIfPossible(method, targetClass, method.getDeclaringClass());
+		}
+		return result;
+	}
+
+	private static Method findInterfaceMethodIfPossible(Method method, Class<?> startClass, Class<?> endClass) {
+		Class<?>[] parameterTypes = null;
+		Class<?> current = startClass;
+		while (current != null && current != endClass) {
+			if (parameterTypes == null) {
+				// Since Method#getParameterTypes() clones the array, we lazily retrieve
+				// and cache parameter types to avoid cloning the array multiple times.
+				parameterTypes = method.getParameterTypes();
+			}
+			for (Class<?> ifc : current.getInterfaces()) {
+				try {
+					return ifc.getMethod(method.getName(), parameterTypes);
+				}
+				catch (NoSuchMethodException ex) {
+					// ignore
+				}
+			}
+			current = current.getSuperclass();
+		}
+		return method;
+	}
+
 	/**
 	 * @see org.junit.platform.commons.support.ReflectionSupport#findMethod(Class, String, String)
 	 */

platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java

@@ -34,8 +34,10 @@
 import static org.junit.platform.commons.util.ReflectionUtils.readFieldValues;
 import static org.junit.platform.commons.util.ReflectionUtils.tryToReadFieldValue;
 
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -258,6 +260,27 @@ void isGeneric() {
 			}
 		}
 
+		/**
+		 * @see <a href="https://github.com/junit-team/junit5/issues/3684">#3684</a>
+		 */
+		@Test
+		void getInterfaceMethodIfPossible() throws Exception {
+			// "anonymous" because it's implemented as an anonymous class.
+			InputStream anonymousInputStream = InputStream.nullInputStream();
+			Class<?> targetType = anonymousInputStream.getClass();
+			assertThat(targetType.isAnonymousClass()).isTrue();
+
+			Method method = targetType.getMethod("close");
+			assertThat(method).isNotNull();
+			assertThat(method.getDeclaringClass()).isEqualTo(targetType);
+
+			Method interfaceMethod = ReflectionUtils.getInterfaceMethodIfPossible(method, targetType);
+			assertThat(interfaceMethod).isNotNull().isNotEqualTo(method);
+			// InputStream implements Closeable directly, so we find the `close` method
+			// in Closeable instead of AutoCloseable.
+			assertThat(interfaceMethod.getDeclaringClass()).isEqualTo(Closeable.class);
+		}
+
 		static class ClassWithVoidAndNonVoidMethods {
 
 			void voidMethod() {