HHH-9706 Review concurrency of ClassLoaderService and warn against reuse

hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataBuildingProcess.java

@@ -299,8 +299,8 @@ public class MetadataBuildingProcess {
 
 		metadataCollector.processSecondPasses( rootMetadataBuildingContext );
 
-		LinkedHashSet<AdditionalJaxbMappingProducer> producers = classLoaderService.loadJavaServices( AdditionalJaxbMappingProducer.class );
+		Iterable<AdditionalJaxbMappingProducer> producers = classLoaderService.loadJavaServices( AdditionalJaxbMappingProducer.class );
-		if ( producers != null && !producers.isEmpty() ) {
+		if ( producers != null ) {
 			final EntityHierarchyBuilder hierarchyBuilder = new EntityHierarchyBuilder();
 //			final MappingBinder mappingBinder = new MappingBinder( true );
 			// We need to disable validation here.  It seems Envers is not producing valid (according to schema) XML

hibernate-core/src/main/java/org/hibernate/boot/registry/StandardServiceRegistryBuilder.java

@@ -27,7 +27,6 @@ import java.io.File;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 
@@ -316,7 +315,7 @@ public class StandardServiceRegistryBuilder {
 	}
 
 	private void applyServiceContributors() {
-		final LinkedHashSet<ServiceContributor> serviceContributors =
+		final Iterable<ServiceContributor> serviceContributors =
 				bootstrapServiceRegistry.getService( ClassLoaderService.class )
 						.loadJavaServices( ServiceContributor.class );
 

hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/internal/ClassLoaderServiceImpl.java

@@ -30,31 +30,33 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.ServiceLoader;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.boot.registry.classloading.spi.ClassLoadingException;
 import org.hibernate.cfg.AvailableSettings;
 import org.hibernate.internal.CoreLogging;
+import org.hibernate.internal.CoreMessageLogger;
 import org.hibernate.internal.util.ClassLoaderHelper;
 
-import org.jboss.logging.Logger;
-
 /**
  * Standard implementation of the service for interacting with class loaders
  *
  * @author Steve Ebersole
+ * @author Sanne Grinovero
  */
 public class ClassLoaderServiceImpl implements ClassLoaderService {
-	private static final Logger log = CoreLogging.logger( ClassLoaderServiceImpl.class );
 
-	private final Map<Class, ServiceLoader> serviceLoaders = new HashMap<Class, ServiceLoader>();
+	private static final CoreMessageLogger log = CoreLogging.messageLogger( ClassLoaderServiceImpl.class );
-	private AggregatedClassLoader aggregatedClassLoader;
+
+	private final ConcurrentMap<Class, ServiceLoader> serviceLoaders = new ConcurrentHashMap<Class, ServiceLoader>();
+	private volatile AggregatedClassLoader aggregatedClassLoader;
 
 	/**
 	 * Constructs a ClassLoaderServiceImpl with standard set-up
@@ -172,7 +174,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 	}
 
 	private static class AggregatedClassLoader extends ClassLoader {
-		private ClassLoader[] individualClassLoaders;
+		private final ClassLoader[] individualClassLoaders;
 
 		private AggregatedClassLoader(final LinkedHashSet<ClassLoader> orderedClassLoaderSet) {
 			super( null );
@@ -229,16 +231,13 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 			throw new ClassNotFoundException( "Could not load requested class : " + name );
 		}
 
-		public void destroy() {
-			individualClassLoaders = null;
-		}
 	}
 
 	@Override
 	@SuppressWarnings({"unchecked"})
 	public <T> Class<T> classForName(String className) {
 		try {
-			return (Class<T>) Class.forName( className, true, aggregatedClassLoader );
+			return (Class<T>) Class.forName( className, true, getAggregatedClassLoader() );
 		}
 		catch (Exception e) {
 			throw new ClassLoadingException( "Unable to load class [" + className + "]", e );
@@ -255,7 +254,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 		}
 
 		try {
-			return aggregatedClassLoader.getResource( name );
+			return getAggregatedClassLoader().getResource( name );
 		}
 		catch (Exception ignore) {
 		}
@@ -275,7 +274,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 
 		try {
 			log.tracef( "trying via [ClassLoader.getResourceAsStream(\"%s\")]", name );
-			final InputStream stream = aggregatedClassLoader.getResourceAsStream( name );
+			final InputStream stream = getAggregatedClassLoader().getResourceAsStream( name );
 			if ( stream != null ) {
 				return stream;
 			}
@@ -295,7 +294,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 
 			try {
 				log.tracef( "trying via [ClassLoader.getResourceAsStream(\"%s\")]", stripped );
-				final InputStream stream = aggregatedClassLoader.getResourceAsStream( stripped );
+				final InputStream stream = getAggregatedClassLoader().getResourceAsStream( stripped );
 				if ( stream != null ) {
 					return stream;
 				}
@@ -311,7 +310,7 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 	public List<URL> locateResources(String name) {
 		final ArrayList<URL> urls = new ArrayList<URL>();
 		try {
-			final Enumeration<URL> urlEnumeration = aggregatedClassLoader.getResources( name );
+			final Enumeration<URL> urlEnumeration = getAggregatedClassLoader().getResources( name );
 			if ( urlEnumeration != null && urlEnumeration.hasMoreElements() ) {
 				while ( urlEnumeration.hasMoreElements() ) {
 					urls.add( urlEnumeration.nextElement() );
@@ -326,16 +325,12 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 
 	@Override
 	@SuppressWarnings("unchecked")
-	public <S> LinkedHashSet<S> loadJavaServices(Class<S> serviceContract) {
+	public <S> Collection<S> loadJavaServices(Class<S> serviceContract) {
-		ServiceLoader<S> serviceLoader;
+		ServiceLoader<S> serviceLoader = serviceLoaders.get( serviceContract );
-		if ( serviceLoaders.containsKey( serviceContract ) ) {
+		if ( serviceLoader == null ) {
-			serviceLoader = serviceLoaders.get( serviceContract );
+			serviceLoader = ServiceLoader.load( serviceContract, getAggregatedClassLoader() );
-		}
-		else {
-			serviceLoader = ServiceLoader.load( serviceContract, aggregatedClassLoader );
 			serviceLoaders.put( serviceContract, serviceLoader );
 		}
-
 		final LinkedHashSet<S> services = new LinkedHashSet<S>();
 		for ( S service : serviceLoader ) {
 			services.add( service );
@@ -343,17 +338,23 @@ public class ClassLoaderServiceImpl implements ClassLoaderService {
 		return services;
 	}
 
+
+	private ClassLoader getAggregatedClassLoader() {
+		final ClassLoader aggregated = this.aggregatedClassLoader;
+		if ( aggregated == null ) {
+			throw log.usingStoppedClassLoaderService();
+		}
+		return aggregated;
+	}
+
 	@Override
 	public void stop() {
 		for ( ServiceLoader serviceLoader : serviceLoaders.values() ) {
 			serviceLoader.reload(); // clear service loader providers
 		}
 		serviceLoaders.clear();
-
+		//Avoid ClassLoader leaks
-		if ( aggregatedClassLoader != null ) {
+		this.aggregatedClassLoader = null;
-			aggregatedClassLoader.destroy();
-			aggregatedClassLoader = null;
-		}
 	}
 
 }

hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/spi/ClassLoaderService.java

@@ -25,7 +25,7 @@ package org.hibernate.boot.registry.classloading.spi;
 
 import java.io.InputStream;
 import java.net.URL;
-import java.util.LinkedHashSet;
+import java.util.Collection;
 import java.util.List;
 
 import org.hibernate.service.Service;
@@ -87,5 +87,5 @@ public interface ClassLoaderService extends Service, Stoppable {
 	 *     
 	 * @return The ordered set of discovered services.
 	 */
-	public <S> LinkedHashSet<S> loadJavaServices(Class<S> serviceContract);
+	public <S> Collection<S> loadJavaServices(Class<S> serviceContract);
 }

hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java

@@ -1700,4 +1700,6 @@ public interface CoreMessageLogger extends BasicLogger {
 	@Message(value = "Unable to interpret type [%s] as an AttributeConverter due to an exception : %s", id = 468)
 	void logBadHbmAttributeConverterType(String type, String exceptionMessage);
 
+	@Message(value = "The ClassLoaderService can not be reused. This instance was stopped already.", id = 469)
+	HibernateException usingStoppedClassLoaderService();
 }

hibernate-core/src/test/java/org/hibernate/test/service/ClassLoaderServiceImplTest.java

@@ -5,9 +5,9 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 import java.util.Enumeration;
-import java.util.LinkedHashSet;
 import javax.persistence.Entity;
 
+import org.hibernate.HibernateException;
 import org.hibernate.boot.registry.BootstrapServiceRegistryBuilder;
 import org.hibernate.boot.registry.StandardServiceRegistryBuilder;
 import org.hibernate.boot.registry.classloading.internal.ClassLoaderServiceImpl;
@@ -15,13 +15,11 @@ import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.integrator.spi.Integrator;
 import org.hibernate.internal.util.ConfigHelper;
 import org.hibernate.service.ServiceRegistry;
-
 import org.hibernate.testing.TestForIssue;
 import org.junit.Assert;
 import org.junit.Test;
 
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 
 /**
@@ -76,21 +74,25 @@ public class ClassLoaderServiceImplTest {
     	
     	StandardServiceRegistryBuilder.destroy( serviceRegistry );
     	
-    	// Should return null -- aggregratedClassLoader blown away.
+    	try {
-    	testIntegrator2 = findTestIntegrator( classLoaderService );
+    		findTestIntegrator( classLoaderService );
-    	assertNull( testIntegrator2 );
+    		Assert.fail("Should have thrown an HibernateException -- the ClassLoaderService instance was closed.");
-    }
+    	}
-    
+    	catch (HibernateException e) {
-    private TestIntegrator findTestIntegrator(ClassLoaderService classLoaderService) {
+    		String message = e.getMessage();
-    	final LinkedHashSet<Integrator> integrators = classLoaderService.loadJavaServices( Integrator.class );
+    		Assert.assertEquals( "HHH000469: The ClassLoaderService can not be reused. This instance was stopped already.", message);
-    	for (Integrator integrator : integrators) {
-    		if (integrator instanceof TestIntegrator) {
-    			return (TestIntegrator) integrator;
-    		}
     	}
-    	return null;
     }
 
+	private TestIntegrator findTestIntegrator(ClassLoaderService classLoaderService) {
+		for ( Integrator integrator : classLoaderService.loadJavaServices( Integrator.class ) ) {
+			if ( integrator instanceof TestIntegrator ) {
+				return (TestIntegrator) integrator;
+			}
+		}
+		return null;
+	}
+
     private static class TestClassLoader extends ClassLoader {
     	
     	/**

hibernate-osgi/src/main/java/org/hibernate/osgi/OSGiClassLoaderServiceImpl.java

@@ -20,7 +20,6 @@
  */
 package org.hibernate.osgi;
 
-import java.util.Collections;
 import java.util.LinkedHashSet;
 
 import org.hibernate.boot.registry.classloading.internal.ClassLoaderServiceImpl;
@@ -45,10 +44,16 @@ public class OSGiClassLoaderServiceImpl extends ClassLoaderServiceImpl implement
 
 	@Override
 	public <S> LinkedHashSet<S> loadJavaServices(Class<S> serviceContract) {
-		LinkedHashSet<S> parentDiscoveredServices = super.loadJavaServices( serviceContract );
+		Iterable<S> parentDiscoveredServices = super.loadJavaServices( serviceContract );
 		S[] serviceImpls = osgiServiceUtil.getServiceImpls(serviceContract);
-		Collections.addAll( parentDiscoveredServices, serviceImpls );
+		LinkedHashSet<S> composite = new LinkedHashSet<S>();
-		return parentDiscoveredServices;
+		for ( S service : parentDiscoveredServices ) {
+			composite.add( service );
+		}
+		for ( S service : serviceImpls ) {
+			composite.add( service );
+		}
+		return composite;
 	}
 
 	@Override