From e7cefd8ddd00eff6a4614508d5bb0ee15bfd9aeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Mon, 27 May 2019 09:37:56 +0200 Subject: [PATCH] HHH-13409 Rework AggregatedServiceLoader to minimize the risk of regression In particular: * Keep the old behavior when retrieving services on JDK8 * On JDK9+, query the AggregatedClassLoader first (before individual class loaders) when retrieving services. * On JDK9+, use ServiceLoader.Provider to avoid instantiating services a second time if we know we already instantiated them with another class loader. --- .../internal/AggregatedServiceLoader.java | 258 ++++++++++++++---- .../internal/ClassLoaderServiceImpl.java | 2 +- 2 files changed, 205 insertions(+), 55 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/AggregatedServiceLoader.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/AggregatedServiceLoader.java index 891c4cb5e9..eead6d561a 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/AggregatedServiceLoader.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/AggregatedServiceLoader.java @@ -6,86 +6,236 @@ */ package org.hibernate.boot.registry.classloading.internal; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.ServiceLoader; import java.util.Set; +import java.util.function.Supplier; +import java.util.stream.Stream; + +import org.hibernate.AssertionFailure; /** * A service loader bound to an {@link AggregatedClassLoader}. - *

- * When retrieving services from providers in the module path, - * the service loader internally uses a map from classloader to service catalog. - * Since the aggregated class loader is artificial and unknown from the service loader, - * it will never match any service from the module path. - *

- * To work around this problem, - * we instantiate one service loader per individual class loader and get services from these. - * This could result in duplicates, so we take specific care to avoid using the same service provider twice. - * See {@link #getAll()}. - *

- * Note that, in the worst case, - * the service retrieved from each individual class loader could load duplicate versions - * of classes already loaded from another class loader. - * For example in an aggregated class loader made up of individual class loader A, B, C: - * it is possible that class C1 was already loaded from A, - * but then we load service S1 from B, and this service will also need class C1 but won't find it in class loader B, - * so it will load its own version of that class. - *

- * We assume that this situation will never occur in practice because class loaders - * are structure in a hierarchy that prevents one class to be loaded twice. - * * @param The type of the service contract. */ -final class AggregatedServiceLoader { +abstract class AggregatedServiceLoader { - private final List> delegates; + private static final Method SERVICE_LOADER_STREAM_METHOD; + private static final Method PROVIDER_TYPE_METHOD; - AggregatedServiceLoader(AggregatedClassLoader aggregatedClassLoader, Class serviceContract) { - this.delegates = new ArrayList<>(); - final Iterator clIterator = aggregatedClassLoader.newClassLoaderIterator(); - while ( clIterator.hasNext() ) { - this.delegates.add( - ServiceLoader.load( serviceContract, clIterator.next() ) - ); + static { + Class serviceLoaderClass = ServiceLoader.class; + Method serviceLoaderStreamMethod = null; + Method providerTypeMethod = null; + try { + /* + * JDK 9 introduced the stream() method on ServiceLoader, + * which we need in order to avoid duplicate service instantiation. + * See ClassPathAndModulePathAggregatedServiceLoader. + */ + serviceLoaderStreamMethod = serviceLoaderClass.getMethod( "stream" ); + Class providerClass = Class.forName( serviceLoaderClass.getName() + "$Provider" ); + providerTypeMethod = providerClass.getMethod( "type" ); + } + catch (NoSuchMethodException | ClassNotFoundException e) { + /* + * Probably Java 8. + * Leave the method constants null, + * we will automatically use a service loader implementation that doesn't rely on them. + * See create(...). + */ + } + + SERVICE_LOADER_STREAM_METHOD = serviceLoaderStreamMethod; + PROVIDER_TYPE_METHOD = providerTypeMethod; + } + + static AggregatedServiceLoader create(AggregatedClassLoader aggregatedClassLoader, + Class serviceContract) { + if ( SERVICE_LOADER_STREAM_METHOD != null ) { + // Java 9+ + return new ClassPathAndModulePathAggregatedServiceLoader<>( aggregatedClassLoader, serviceContract ); + } + else { + // Java 8 + return new ClassPathOnlyAggregatedServiceLoader<>( aggregatedClassLoader, serviceContract ); } } /** * @return All the loaded services. */ - public Collection getAll() { - Set alreadyEncountered = new HashSet<>(); - Set result = new LinkedHashSet<>(); - for ( ServiceLoader delegate : delegates ) { - for ( S service : delegate ) { - Class type = service.getClass(); - String typeName = type.getName(); - /* - * We may encounter the same service provider multiple times, - * because the individual class loaders may give access to the same types - * (at the very least a single class loader may be present twice in the aggregated class loader). - * However, we only want to get the service from each provider once. - */ - if ( alreadyEncountered.add( typeName ) ) { - result.add( service ); - } - } - } - return result; - } + public abstract Collection getAll(); /** * Release all resources. */ - public void close() { - // Clear service providers - for ( ServiceLoader delegate : delegates ) { + public abstract void close(); + + /** + * An {@link AggregatedServiceLoader} that will only detect services defined in the class path, + * not in the module path, + * because it passes the aggregated classloader directly to the service loader. + *

+ * This implementation is best when running Hibernate ORM on Java 8. + * On Java 9 and above, {@link ClassPathAndModulePathAggregatedServiceLoader} should be used. + * + * @param The type of loaded services. + */ + private static class ClassPathOnlyAggregatedServiceLoader extends AggregatedServiceLoader { + private final ServiceLoader delegate; + + private ClassPathOnlyAggregatedServiceLoader(AggregatedClassLoader aggregatedClassLoader, Class serviceContract) { + this.delegate = ServiceLoader.load( serviceContract, aggregatedClassLoader ); + } + + @Override + public Collection getAll() { + final Set services = new LinkedHashSet<>(); + for ( S service : delegate ) { + services.add( service ); + } + return services; + } + + @Override + public void close() { + // Clear service providers delegate.reload(); } } + + /** + * An {@link AggregatedServiceLoader} that will detect services defined in the class path or in the module path. + *

+ * This implementation only works when running Hibernate ORM on Java 9 and above. + * On Java 8, {@link ClassPathOnlyAggregatedServiceLoader} must be used. + *

+ * When retrieving services from providers in the module path, + * the service loader internally uses a map from classloader to service catalog. + * Since the aggregated class loader is artificial and unknown from the service loader, + * it will never match any service from the module path. + *

+ * To work around this problem, + * we try to get services from a service loader bound to the aggregated class loader first, + * then we try a service loader bound to each individual class loader. + *

+ * This could result in duplicates, so we take specific care to avoid using the same service provider twice. + * See {@link #getAll()}. + *

+ * Note that, in the worst case, + * the service retrieved from each individual class loader could load duplicate versions + * of classes already loaded from another class loader. + * For example in an aggregated class loader made up of individual class loader A, B, C: + * it is possible that class C1 was already loaded from A, + * but then we load service S1 from B, and this service will also need class C1 but won't find it in class loader B, + * so it will load its own version of that class. + *

+ * We assume that this situation will never occur in practice because class loaders + * are structure in a hierarchy that prevents one class to be loaded twice. + * + * @param The type of loaded services. + */ + private static class ClassPathAndModulePathAggregatedServiceLoader extends AggregatedServiceLoader { + private final List> delegates; + private Collection cache = null; + + private ClassPathAndModulePathAggregatedServiceLoader(AggregatedClassLoader aggregatedClassLoader, + Class serviceContract) { + this.delegates = new ArrayList<>(); + // Always try the aggregated class loader first + this.delegates.add( ServiceLoader.load( serviceContract, aggregatedClassLoader ) ); + + // Then also try the individual class loaders, + // because only them can instantiate services provided by jars in the module path + final Iterator clIterator = aggregatedClassLoader.newClassLoaderIterator(); + while ( clIterator.hasNext() ) { + this.delegates.add( + ServiceLoader.load( serviceContract, clIterator.next() ) + ); + } + } + + @Override + public Collection getAll() { + if ( cache == null ) { + /* + * loadAll() uses ServiceLoader.Provider.get(), which doesn't cache the instance internally, + * on contrary to ServiceLoader.iterator(). + * Looking at how https://hibernate.atlassian.net/browse/HHH-8363 was solved, + * waiting for Hibernate ORM to shut down before clearing the service caches, + * it seems caching of service instances is important, or at least used to be important. + * Thus we cache service instances ourselves to avoid any kind of backward-incompatibility. + * If one day we decide caching isn't important, this cache can be removed, + * as well as the close() method, + * and also the service loader map in ClassLoaderServiceImpl, + * and we can simply call .reload() on the service loader after we load services + * in ClassPathOnlyAggregatedServiceLoader.getAll(). + */ + cache = Collections.unmodifiableCollection( loadAll() ); + } + return cache; + } + + @SuppressWarnings("unchecked") + private Collection loadAll() { + Set alreadyEncountered = new HashSet<>(); + Set result = new LinkedHashSet<>(); + delegates.stream() + // Each loader's stream() method returns a stream of service providers: flatten these into a single stream + .flatMap( delegate -> { + try { + return (Stream>) SERVICE_LOADER_STREAM_METHOD.invoke( delegate ); + } + catch (RuntimeException | IllegalAccessException | InvocationTargetException e) { + throw new AssertionFailure( "Error calling ServiceLoader.stream()", e ); + } + } ) + // For each provider, check its type to be sure we don't use a provider twice, then get the service + .forEach( provider -> { + Class type; + try { + type = (Class) PROVIDER_TYPE_METHOD.invoke( provider ); + } + catch (RuntimeException | IllegalAccessException | InvocationTargetException e) { + throw new AssertionFailure( "Error calling ServiceLoader.Provider.type()", e ); + } + String typeName = type.getName(); + /* + * We may encounter the same service provider multiple times, + * because the individual class loaders may give access to the same types + * (at the very least a single class loader may be present twice in the aggregated class loader). + * However, we only want to get the service from each provider once. + * + * ServiceLoader.stream() is useful in that regard, + * since it allows us to check the type of the service provider + * before the service is even instantiated. + * + * We could just instantiate every service and check their type afterwards, + * but 1. it would lead to unnecessary instantiation which could have side effects, + * in particular regarding class loading, + * and 2. the type of the provider may not always be the type of the service, + * and one provider may return different types of services + * depending on conditions known only to itself. + */ + if ( alreadyEncountered.add( typeName ) ) { + result.add( provider.get() ); + } + } ); + return result; + } + + @Override + public void close() { + cache = null; + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/ClassLoaderServiceImpl.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/ClassLoaderServiceImpl.java index e69234af7d..b3d607f35d 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/ClassLoaderServiceImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/ClassLoaderServiceImpl.java @@ -247,7 +247,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService { public Collection loadJavaServices(Class serviceContract) { AggregatedServiceLoader serviceLoader = (AggregatedServiceLoader) serviceLoaders.get( serviceContract ); if ( serviceLoader == null ) { - serviceLoader = new AggregatedServiceLoader( getAggregatedClassLoader(), serviceContract ); + serviceLoader = AggregatedServiceLoader.create( getAggregatedClassLoader(), serviceContract ); serviceLoaders.put( serviceContract, serviceLoader ); } return serviceLoader.getAll();