Avoid calling ServiceLoader twice with same class loader (#5948)

* Avoid calling ServiceLoader twice with same class loader (#5903)

* Avoid calling ServiceLoader twice with same class loader

The ServiceLoader goes through all the jars to find its files and it
seems like a good idea to avoid doing it twice for the same class
loader.

This is somehow related to the work we are doing in Quarkus for Leyden.

* Simplify io.vertx.core.impl.ServiceHelper.loadFactories(java.lang.Class<T>, java.lang.ClassLoader)

For readability

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

* Skip reloading factories if TCCL happens to be the same as the ServiceHelper's CL

Saves the cost of scanning the classpath again

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

* Verify we don't try to get resource from CL twice when the TCCL is the same as the ServiceHelper's classloader

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

---------

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
Co-authored-by: Thomas Segismont <tsegismont@gmail.com>

* Fix test ServiceCommandLoaderTest.testNoCommandsWhenLoadedFromEmptyClassloader

Bug was previously hidden by an incorrect test...

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

---------

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
Co-authored-by: Guillaume Smet <guillaume.smet@gmail.com>

Author: tsegismont

src/main/java/io/vertx/core/ServiceHelper.java

@@ -11,7 +11,10 @@
 
 package io.vertx.core;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.ServiceLoader;
 
 /**
  * A helper class for loading factories from the classpath.
@@ -45,29 +48,26 @@ public static <T> Collection<T> loadFactories(Class<T> clazz) {
   }
 
   public static <T> Collection<T> loadFactories(Class<T> clazz, ClassLoader classLoader) {
-    List<T> list = new ArrayList<>();
-    ServiceLoader<T> factories;
     if (classLoader != null) {
-      factories = ServiceLoader.load(clazz, classLoader);
-    } else {
-      // this is equivalent to:
-      // ServiceLoader.load(clazz, TCCL);
-      factories = ServiceLoader.load(clazz);
+      return loadFactories(ServiceLoader.load(clazz, classLoader));
+    }
+    classLoader = Thread.currentThread().getContextClassLoader();
+    List<T> list = loadFactories(ServiceLoader.load(clazz, classLoader));
+    if (list.isEmpty() && classLoader != ServiceHelper.class.getClassLoader()) {
+      // By default, ServiceLoader.load uses the TCCL, this may not be enough in environment dealing with
+      // classloaders differently such as OSGi. So we should try to use the  classloader having loaded this
+      // class. In OSGi it would be the bundle exposing vert.x and so have access to all its classes.
+      list = loadFactories(ServiceLoader.load(clazz, ServiceHelper.class.getClassLoader()));
     }
+    return list;
+  }
+
+  private static <T> List<T> loadFactories(ServiceLoader<T> factories) {
+    List<T> list = new ArrayList<>();
     if (factories.iterator().hasNext()) {
       factories.iterator().forEachRemaining(list::add);
       return list;
-    } else {
-      // By default ServiceLoader.load uses the TCCL, this may not be enough in environment dealing with
-      // classloaders differently such as OSGi. So we should try to use the  classloader having loaded this
-      // class. In OSGi it would be the bundle exposing vert.x and so have access to all its classes.
-      factories = ServiceLoader.load(clazz, ServiceHelper.class.getClassLoader());
-      if (factories.iterator().hasNext()) {
-        factories.iterator().forEachRemaining(list::add);
-        return list;
-      } else {
-        return Collections.emptyList();
-      }
     }
+    return list;
   }
 }

src/test/java/io/vertx/core/ServiceHelperTest.java

@@ -18,12 +18,16 @@
 import org.junit.Test;
 
 import java.io.File;
+import java.io.IOException;
 import java.lang.reflect.Method;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Collection;
+import java.util.Enumeration;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertEquals;
 
 /**
  * Check the service helper behavior.
@@ -128,4 +132,43 @@ public void loadFactoriesWithVertxClassloader() throws Exception {
     Collection collection = (Collection) method.invoke(null, someFactoryClass);
     assertThat(collection).hasSize(1);
   }
+
+  @Test
+  public void shouldInvokeGetResourcesOnce() throws Exception {
+    // We want to make sure we don't try to get resource from CL twice
+    // When the TCCL is the same as the ServiceHelper's classloader
+
+    AtomicInteger invocations = new AtomicInteger();
+
+    // Load the ServiceHelper class from a custom classloader.
+    ClassLoader custom = new URLClassLoader(new URL[]{
+      new File(TestUtils.MAVEN_TARGET_DIR, "classes").toURI().toURL(),
+      new File(TestUtils.MAVEN_TARGET_DIR, "test-classes").toURI().toURL(),
+      serviceHelperFile.toURI().toURL(),
+    }, null) {
+
+      @Override
+      public Enumeration<URL> getResources(String name) throws IOException {
+        invocations.incrementAndGet();
+        return super.getResources(name);
+      }
+    };
+
+    Class serviceHelperClass = custom.loadClass(ServiceHelper.class.getName());
+    Class notImplementedSPIClass = custom.loadClass(NotImplementedSPI.class.getName());
+    assertThat(serviceHelperClass.getClassLoader()).isEqualTo(custom);
+    assertThat(notImplementedSPIClass.getClassLoader()).isEqualTo(custom);
+
+    final ClassLoader originalTCCL = Thread.currentThread().getContextClassLoader();
+    try {
+      // Set TCCL to the CL that loaded the ServiceHelper class
+      Thread.currentThread().setContextClassLoader(custom);
+      Method method = serviceHelperClass.getMethod("loadFactories", Class.class);
+      Collection collection = (Collection) method.invoke(null, notImplementedSPIClass);
+      assertThat(collection).hasSize(0);
+      assertEquals(1, invocations.get());
+    } finally {
+      Thread.currentThread().setContextClassLoader(originalTCCL);
+    }
+  }
 }

src/test/java/io/vertx/core/impl/launcher/ServiceCommandLoaderTest.java

@@ -55,7 +55,7 @@ public void testNoCommandsWhenLoadedFromEmptyClassloader() {
 
     // We see the implementation from the classpath
     loader = new ServiceCommandFactoryLoader(classLoader);
-    assertThat(loader.lookup()).isNotEmpty();
+    assertThat(loader.lookup()).isEmpty();
   }