Skip to content

Commit

Permalink
Revert "Reapply "Make FacadeClassLoader a singleton""
Browse files Browse the repository at this point in the history
This reverts commit ff60a3a.
  • Loading branch information
holly-cummins committed Jan 15, 2025
1 parent e76475a commit bfd590d
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,17 @@ private <T> T nintercept(Invocation<T> 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());
try {
// 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);
Expand Down

0 comments on commit bfd590d

Please sign in to comment.