From daf120b063bc0be40f27cce92ce84f1be629cd4b Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Thu, 20 Mar 2014 14:47:57 -0500 Subject: [PATCH] HHH-8923 - Reconsider closing of ServiceRegistry instances --- .../BootstrapServiceRegistryImpl.java | 49 +++++++++++-- .../internal/StandardServiceRegistryImpl.java | 9 --- .../internal/AbstractServiceRegistryImpl.java | 70 ++++++++++++++++--- .../spi/ServiceRegistryImplementor.java | 12 ++++ .../ServiceRegistryClosingCascadeTest.java | 52 ++++++++++++++ .../junit4/BaseCoreFunctionalTestCase.java | 15 ++-- 6 files changed, 181 insertions(+), 26 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java index d6d5b6b13c..d0515a7937 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java @@ -23,7 +23,9 @@ */ package org.hibernate.boot.registry.internal; +import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.Set; import org.hibernate.boot.registry.BootstrapServiceRegistry; import org.hibernate.boot.registry.classloading.internal.ClassLoaderServiceImpl; @@ -33,6 +35,7 @@ import org.hibernate.boot.registry.selector.spi.StrategySelector; import org.hibernate.integrator.internal.IntegratorServiceImpl; import org.hibernate.integrator.spi.Integrator; import org.hibernate.integrator.spi.IntegratorService; +import org.hibernate.internal.CoreLogging; import org.hibernate.internal.CoreMessageLogger; import org.hibernate.service.Service; import org.hibernate.service.ServiceRegistry; @@ -61,17 +64,18 @@ import org.jboss.logging.Logger; public class BootstrapServiceRegistryImpl implements ServiceRegistryImplementor, BootstrapServiceRegistry, ServiceBinding.ServiceLifecycleOwner { - private static final CoreMessageLogger LOG = Logger.getMessageLogger( - CoreMessageLogger.class, - BootstrapServiceRegistryImpl.class.getName() - ); - + private static final CoreMessageLogger LOG = CoreLogging.messageLogger( BootstrapServiceRegistryImpl.class ); + + private boolean active = true; + private static final LinkedHashSet NO_INTEGRATORS = new LinkedHashSet(); private final ServiceBinding classLoaderServiceBinding; private final ServiceBinding strategySelectorBinding; private final ServiceBinding integratorServiceBinding; + private Set childRegistries; + /** * Constructs a BootstrapServiceRegistryImpl. * @@ -180,6 +184,10 @@ public class BootstrapServiceRegistryImpl @Override public void destroy() { + if ( !active ) { + return; + } + active = false; destroy( classLoaderServiceBinding ); destroy( strategySelectorBinding ); destroy( integratorServiceBinding ); @@ -189,6 +197,10 @@ public class BootstrapServiceRegistryImpl serviceBinding.getLifecycleOwner().stopService( serviceBinding ); } + public boolean isActive() { + return active; + } + @Override public ServiceRegistry getParentServiceRegistry() { return null; @@ -227,4 +239,31 @@ public class BootstrapServiceRegistryImpl } } + @Override + public void registerChild(ServiceRegistryImplementor child) { + if ( childRegistries == null ) { + childRegistries = new HashSet(); + } + if ( !childRegistries.add( child ) ) { + LOG.warnf( + "Child ServiceRegistry [%s] was already registered; this will end badly later...", + child + ); + } + } + + @Override + public void deRegisterChild(ServiceRegistryImplementor child) { + if ( childRegistries == null ) { + throw new IllegalStateException( "No child ServiceRegistry registrations found" ); + } + childRegistries.remove( child ); + if ( childRegistries.isEmpty() ) { + LOG.debug( + "Implicitly destroying Boot-strap registry on de-registration " + + "of all child ServiceRegistries" + ); + destroy(); + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java b/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java index 29ed9b56f3..137d35b2ee 100644 --- a/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java @@ -35,7 +35,6 @@ import org.hibernate.service.internal.ProvidedService; import org.hibernate.service.spi.Configurable; import org.hibernate.service.spi.ServiceBinding; import org.hibernate.service.spi.ServiceInitiator; -import org.hibernate.service.spi.ServiceRegistryImplementor; /** * Standard Hibernate implementation of the standard service registry. @@ -89,12 +88,4 @@ public class StandardServiceRegistryImpl extends AbstractServiceRegistryImpl imp ( (Configurable) serviceBinding.getService() ).configure( configurationValues ); } } - - @Override - public void destroy() { - super.destroy(); - if ( getParentServiceRegistry() != null ) { - ( (ServiceRegistryImplementor) getParentServiceRegistry() ).destroy(); - } - } } diff --git a/hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java b/hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java index d24f3aafcc..ca7716d8b0 100644 --- a/hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java @@ -24,8 +24,10 @@ package org.hibernate.service.internal; import java.lang.reflect.Method; +import java.util.HashSet; import java.util.List; import java.util.ListIterator; +import java.util.Set; import org.hibernate.boot.registry.BootstrapServiceRegistry; import org.hibernate.cfg.Environment; @@ -70,6 +72,8 @@ public abstract class AbstractServiceRegistryImpl // assume 20 services for initial sizing private final List serviceBindingList = CollectionHelper.arrayList( 20 ); + private Set childRegistries; + @SuppressWarnings( {"UnusedDeclaration"}) protected AbstractServiceRegistryImpl() { this( (ServiceRegistryImplementor) null ); @@ -78,6 +82,8 @@ public abstract class AbstractServiceRegistryImpl protected AbstractServiceRegistryImpl(ServiceRegistryImplementor parent) { this.parent = parent; this.allowCrawling = ConfigurationHelper.getBoolean( ALLOW_CRAWLING, Environment.getProperties(), true ); + + this.parent.registerChild( this ); } public AbstractServiceRegistryImpl(BootstrapServiceRegistry bootstrapServiceRegistry) { @@ -86,6 +92,8 @@ public abstract class AbstractServiceRegistryImpl } this.parent = (ServiceRegistryImplementor) bootstrapServiceRegistry; this.allowCrawling = ConfigurationHelper.getBoolean( ALLOW_CRAWLING, Environment.getProperties(), true ); + + this.parent.registerChild( this ); } @SuppressWarnings({ "unchecked" }) @@ -309,18 +317,36 @@ public abstract class AbstractServiceRegistryImpl } } + private boolean active = true; + + public boolean isActive() { + return active; + } + @Override @SuppressWarnings( {"unchecked"}) public void destroy() { - synchronized ( serviceBindingList ) { - ListIterator serviceBindingsIterator = serviceBindingList.listIterator( serviceBindingList.size() ); - while ( serviceBindingsIterator.hasPrevious() ) { - final ServiceBinding serviceBinding = serviceBindingsIterator.previous(); - serviceBinding.getLifecycleOwner().stopService( serviceBinding ); - } - serviceBindingList.clear(); + if ( !active ) { + return; + } + + active = false; + try { + synchronized (serviceBindingList) { + ListIterator serviceBindingsIterator = serviceBindingList.listIterator( + serviceBindingList.size() + ); + while ( serviceBindingsIterator.hasPrevious() ) { + final ServiceBinding serviceBinding = serviceBindingsIterator.previous(); + serviceBinding.getLifecycleOwner().stopService( serviceBinding ); + } + serviceBindingList.clear(); + } + serviceBindingMap.clear(); + } + finally { + parent.deRegisterChild( this ); } - serviceBindingMap.clear(); } @Override @@ -335,4 +361,32 @@ public abstract class AbstractServiceRegistryImpl } } } + + @Override + public void registerChild(ServiceRegistryImplementor child) { + if ( childRegistries == null ) { + childRegistries = new HashSet(); + } + if ( !childRegistries.add( child ) ) { + log.warnf( + "Child ServiceRegistry [%s] was already registered; this will end badly later...", + child + ); + } + } + + @Override + public void deRegisterChild(ServiceRegistryImplementor child) { + if ( childRegistries == null ) { + throw new IllegalStateException( "No child ServiceRegistry registrations found" ); + } + childRegistries.remove( child ); + if ( childRegistries.isEmpty() ) { + log.debug( + "Implicitly destroying ServiceRegistry on de-registration " + + "of all child ServiceRegistries" + ); + destroy(); + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceRegistryImplementor.java b/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceRegistryImplementor.java index d2a5e59046..56cb45b58e 100644 --- a/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceRegistryImplementor.java +++ b/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceRegistryImplementor.java @@ -46,4 +46,16 @@ public interface ServiceRegistryImplementor extends ServiceRegistry { * Release resources */ public void destroy(); + + /** + * When a registry is created with a parent, the parent is notified of the child + * via this callback. + */ + public void registerChild(ServiceRegistryImplementor child); + + /** + * When a registry is created with a parent, the parent is notified of the child + * via this callback. + */ + public void deRegisterChild(ServiceRegistryImplementor child); } diff --git a/hibernate-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java b/hibernate-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java new file mode 100644 index 0000000000..7eb792620f --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java @@ -0,0 +1,52 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * Copyright (c) 2014, Red Hat Inc. or third-party contributors as + * indicated by the @author tags or express copyright attribution + * statements applied by the authors. All third-party contributions are + * distributed under license by Red Hat Inc. + * + * This copyrighted material is made available to anyone wishing to use, modify, + * copy, or redistribute it subject to the terms and conditions of the GNU + * Lesser General Public License, as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this distribution; if not, write to: + * Free Software Foundation, Inc. + * 51 Franklin Street, Fifth Floor + * Boston, MA 02110-1301 USA + */ +package org.hibernate.service.internal; + +import org.hibernate.SessionFactory; +import org.hibernate.boot.registry.BootstrapServiceRegistry; +import org.hibernate.boot.registry.BootstrapServiceRegistryBuilder; +import org.hibernate.boot.registry.internal.BootstrapServiceRegistryImpl; +import org.hibernate.metamodel.MetadataSources; + +import org.hibernate.testing.junit4.BaseUnitTestCase; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * @author Steve Ebersole + */ +public class ServiceRegistryClosingCascadeTest extends BaseUnitTestCase { + @Test + public void testSessionFactoryClosing() { + BootstrapServiceRegistry bsr = new BootstrapServiceRegistryBuilder().build(); + assertTrue( ( (BootstrapServiceRegistryImpl) bsr ).isActive() ); + MetadataSources metadataSources = new MetadataSources( bsr ); + SessionFactory sf = metadataSources.buildMetadata().buildSessionFactory(); + + sf.close(); + assertFalse( ( (BootstrapServiceRegistryImpl) bsr ).isActive() ); + } +} diff --git a/hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java b/hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java index 59aa746f5f..64da3a4662 100644 --- a/hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java +++ b/hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java @@ -70,6 +70,7 @@ import org.hibernate.testing.cache.CachingRegionFactory; import org.junit.After; import org.junit.Before; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.fail; /** @@ -404,10 +405,16 @@ public abstract class BaseCoreFunctionalTestCase extends BaseUnitTestCase { sessionFactory.close(); sessionFactory = null; configuration = null; - if(serviceRegistry == null){ - return; - } - serviceRegistry.destroy(); + if ( serviceRegistry != null ) { + if ( serviceRegistry.isActive() ) { + try { + serviceRegistry.destroy(); + } + catch (Exception ignore) { + } + fail( "StandardServiceRegistry was not closed down as expected" ); + } + } serviceRegistry=null; }