From 4c690a8839113cd6850e1d2952e03a5030be1340 Mon Sep 17 00:00:00 2001 From: Sanne Grinovero Date: Wed, 1 Apr 2015 19:48:45 +0100 Subject: [PATCH] HHH-9706 Review concurrency of ClassLoaderService and warn against reuse --- .../internal/MetadataBuildingProcess.java | 4 +- .../StandardServiceRegistryBuilder.java | 3 +- .../internal/ClassLoaderServiceImpl.java | 57 ++++++++++--------- .../classloading/spi/ClassLoaderService.java | 4 +- .../hibernate/internal/CoreMessageLogger.java | 2 + .../service/ClassLoaderServiceImplTest.java | 32 ++++++----- .../osgi/OSGiClassLoaderServiceImpl.java | 13 +++-- 7 files changed, 62 insertions(+), 53 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataBuildingProcess.java b/hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataBuildingProcess.java index e0b5b24589..55767f5cd6 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataBuildingProcess.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/internal/MetadataBuildingProcess.java @@ -299,8 +299,8 @@ public class MetadataBuildingProcess { metadataCollector.processSecondPasses( rootMetadataBuildingContext ); - LinkedHashSet producers = classLoaderService.loadJavaServices( AdditionalJaxbMappingProducer.class ); - if ( producers != null && !producers.isEmpty() ) { + Iterable producers = classLoaderService.loadJavaServices( AdditionalJaxbMappingProducer.class ); + 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 diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/StandardServiceRegistryBuilder.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/StandardServiceRegistryBuilder.java index 8f810dc062..c6dfb64296 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/StandardServiceRegistryBuilder.java +++ b/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 serviceContributors = + final Iterable serviceContributors = bootstrapServiceRegistry.getService( ClassLoaderService.class ) .loadJavaServices( ServiceContributor.class ); 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 cd4440db2a..f65e78c545 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 @@ -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 serviceLoaders = new HashMap(); - private AggregatedClassLoader aggregatedClassLoader; + private static final CoreMessageLogger log = CoreLogging.messageLogger( ClassLoaderServiceImpl.class ); + + private final ConcurrentMap serviceLoaders = new ConcurrentHashMap(); + 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 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 Class classForName(String className) { try { - return (Class) Class.forName( className, true, aggregatedClassLoader ); + return (Class) 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 locateResources(String name) { final ArrayList urls = new ArrayList(); try { - final Enumeration urlEnumeration = aggregatedClassLoader.getResources( name ); + final Enumeration 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 LinkedHashSet loadJavaServices(Class serviceContract) { - ServiceLoader serviceLoader; - if ( serviceLoaders.containsKey( serviceContract ) ) { - serviceLoader = serviceLoaders.get( serviceContract ); - } - else { - serviceLoader = ServiceLoader.load( serviceContract, aggregatedClassLoader ); + public Collection loadJavaServices(Class serviceContract) { + ServiceLoader serviceLoader = serviceLoaders.get( serviceContract ); + if ( serviceLoader == null ) { + serviceLoader = ServiceLoader.load( serviceContract, getAggregatedClassLoader() ); serviceLoaders.put( serviceContract, serviceLoader ); } - final LinkedHashSet services = new LinkedHashSet(); 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(); - - if ( aggregatedClassLoader != null ) { - aggregatedClassLoader.destroy(); - aggregatedClassLoader = null; - } + //Avoid ClassLoader leaks + this.aggregatedClassLoader = null; } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/spi/ClassLoaderService.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/spi/ClassLoaderService.java index 87a8a2dd9c..e490d3a9f3 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/classloading/spi/ClassLoaderService.java +++ b/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 LinkedHashSet loadJavaServices(Class serviceContract); + public Collection loadJavaServices(Class serviceContract); } diff --git a/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java b/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java index 2f8d2794c4..5d0daef85e 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/CoreMessageLogger.java +++ b/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(); } diff --git a/hibernate-core/src/test/java/org/hibernate/test/service/ClassLoaderServiceImplTest.java b/hibernate-core/src/test/java/org/hibernate/test/service/ClassLoaderServiceImplTest.java index e282562c96..1f83785fa7 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/service/ClassLoaderServiceImplTest.java +++ b/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. - testIntegrator2 = findTestIntegrator( classLoaderService ); - assertNull( testIntegrator2 ); - } - - private TestIntegrator findTestIntegrator(ClassLoaderService classLoaderService) { - final LinkedHashSet integrators = classLoaderService.loadJavaServices( Integrator.class ); - for (Integrator integrator : integrators) { - if (integrator instanceof TestIntegrator) { - return (TestIntegrator) integrator; - } + try { + findTestIntegrator( classLoaderService ); + Assert.fail("Should have thrown an HibernateException -- the ClassLoaderService instance was closed."); + } + catch (HibernateException e) { + String message = e.getMessage(); + Assert.assertEquals( "HHH000469: The ClassLoaderService can not be reused. This instance was stopped already.", message); } - 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 { /** diff --git a/hibernate-osgi/src/main/java/org/hibernate/osgi/OSGiClassLoaderServiceImpl.java b/hibernate-osgi/src/main/java/org/hibernate/osgi/OSGiClassLoaderServiceImpl.java index e7fe9eeb37..2bb441f1b5 100644 --- a/hibernate-osgi/src/main/java/org/hibernate/osgi/OSGiClassLoaderServiceImpl.java +++ b/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 LinkedHashSet loadJavaServices(Class serviceContract) { - LinkedHashSet parentDiscoveredServices = super.loadJavaServices( serviceContract ); + Iterable parentDiscoveredServices = super.loadJavaServices( serviceContract ); S[] serviceImpls = osgiServiceUtil.getServiceImpls(serviceContract); - Collections.addAll( parentDiscoveredServices, serviceImpls ); - return parentDiscoveredServices; + LinkedHashSet composite = new LinkedHashSet(); + for ( S service : parentDiscoveredServices ) { + composite.add( service ); + } + for ( S service : serviceImpls ) { + composite.add( service ); + } + return composite; } @Override