From bfd590d12cfff76a93651dc2c6b064556343eb24 Mon Sep 17 00:00:00 2001 From: Holly Cummins Date: Wed, 15 Jan 2025 10:36:18 +0000 Subject: [PATCH] Revert "Reapply "Make FacadeClassLoader a singleton"" This reverts commit ff60a3ace8ed0c23ebfa6e568e0e4862b10a9485. --- .../dev/testing/JunitTestRunner.java | 2 +- .../junit/classloading/FacadeClassLoader.java | 19 ++++++------------- .../launcher/CustomLauncherInterceptor.java | 4 ++-- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java b/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java index ad4965f8d6b2e..70a81ae6e56d6 100644 --- a/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java +++ b/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java @@ -779,7 +779,7 @@ private DiscoveryResult discoverTestClasses() { // TODO this seems logical, but DOES NOT makes integration-test/test-extension/tests fail ClassLoader parent = ClassLoader.getSystemClassLoader(); System.out.println("HOLLY using parent for facade loader " + parent); - FacadeClassLoader facadeClassLoader = FacadeClassLoader.instance(parent); // TODO ideally it would be in a different module, but that is hard CollaboratingClassLoader.construct(parent); + FacadeClassLoader facadeClassLoader = new FacadeClassLoader(parent); // TODO ideally it would be in a different module, but that is hard CollaboratingClassLoader.construct(parent); facadeClassLoader.setAuxiliaryApplication(true); // TODO clumsy hack, consolidate logic properly diff --git a/core/deployment/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java b/core/deployment/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java index 4c7eaf54188c0..51ec03ef756d8 100644 --- a/core/deployment/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java +++ b/core/deployment/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java @@ -1,5 +1,7 @@ package io.quarkus.test.junit.classloading; +import static io.quarkus.test.common.PathTestHelper.getTestClassesLocation; + import java.io.Closeable; import java.io.File; import java.io.IOException; @@ -93,19 +95,7 @@ public class FacadeClassLoader extends ClassLoader implements Closeable { private boolean isAuxiliaryApplication; private QuarkusClassLoader keyMakerClassLoader; - private static volatile FacadeClassLoader instance; - - // TODO does it make sense to have a parent here when it is sometimes ignored? - // We don't ever want more than one FacadeClassLoader active, especially since config gets initialised on it. - // The gradle test execution can make more than one, perhaps because of its threading model. - public static FacadeClassLoader instance(ClassLoader parent) { - if (instance == null) { - instance = new FacadeClassLoader(parent); - } - return instance; - } - - private FacadeClassLoader(ClassLoader parent) { + public FacadeClassLoader(ClassLoader parent) { // We need to set the super or things don't work on paths which use the maven isolated classloader, such as google cloud functions tests // It seems something in that path is using a method other than loadClass(), and so the inherited method can't do the right thing without a parent super(parent); @@ -201,8 +191,10 @@ public Class loadClass(String name) throws ClassNotFoundException { }) .forEach(System.out::println); } + System.out.println("will try with parent " + parent); try { Class clazz = parent.loadClass(name); + System.out.println("parent found it as " + getTestClassesLocation(clazz)); } catch (ClassNotFoundException e2) { System.out.println("Could not load with the parent " + name); @@ -214,6 +206,7 @@ public Class loadClass(String name) throws ClassNotFoundException { } } + System.out.println("HOLLY canary did load " + name); // TODO should we use JUnit's AnnotationSupport? It searches class hierarchies. Unless we have a good reason not to use it, perhaps we should? // See, for example, https://github.com/marcphilipp/gradle-sandbox/blob/baaa1972e939f5817f54a3d287611cef0601a58d/classloader-per-test-class/src/test/java/org/example/ClassLoaderReplacingLauncherSessionListener.java#L23-L44 Arrays.stream(fromCanary.getAnnotations()) diff --git a/test-framework/junit5/src/main/java/io/quarkus/test/junit/launcher/CustomLauncherInterceptor.java b/test-framework/junit5/src/main/java/io/quarkus/test/junit/launcher/CustomLauncherInterceptor.java index 5b0e6272fbc09..5d3cf07e1ce04 100644 --- a/test-framework/junit5/src/main/java/io/quarkus/test/junit/launcher/CustomLauncherInterceptor.java +++ b/test-framework/junit5/src/main/java/io/quarkus/test/junit/launcher/CustomLauncherInterceptor.java @@ -38,7 +38,6 @@ private T nintercept(Invocation invocation) { ClassLoader old = Thread.currentThread().getContextClassLoader(); // Don't make a facade loader if the JUnitRunner got there ahead of us // they set a runtime classloader so handle that too - // TODO actually, since it's a singleton, we could skip the check if (!(old instanceof FacadeClassLoader)) { System.out.println( "HOLLY intercept constructing a classloader ------------------------------" + Thread.currentThread()); @@ -46,9 +45,10 @@ private T nintercept(Invocation invocation) { // TODO we should be able to do better than this here //TODO We want to tidy up classloaders we created, but not ones created upstream facadeLoader = null; // TODO diagnostics + // TODO should this be a static variable, so we don't make zillions and cause too many files exceptions? // Although in principle we only go through a few times if (facadeLoader == null) { - facadeLoader = FacadeClassLoader.instance(old); // TODO want to do it reflectively CollaboratingClassLoader.construct(old); + facadeLoader = new FacadeClassLoader(old); // TODO want to do it reflectively CollaboratingClassLoader.construct(old); } Thread.currentThread() .setContextClassLoader(facadeLoader);