HHH-8923 - Reconsider closing of ServiceRegistry instances

hibernate · Mar 20, 2014 · bdb27be · bdb27be
1 parent 414a0fb
commit bdb27be
Show file tree

Hide file tree

Showing 6 changed files with 180 additions and 25 deletions.
diff --git a/...core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java b/...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.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 @@
 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<Integrator> NO_INTEGRATORS = new LinkedHashSet<Integrator>();
 
 	private final ServiceBinding<ClassLoaderService> classLoaderServiceBinding;
 	private final ServiceBinding<StrategySelector> strategySelectorBinding;
 	private final ServiceBinding<IntegratorService> integratorServiceBinding;
 
+	private Set<ServiceRegistryImplementor> childRegistries;
+
 	/**
 	 * Constructs a BootstrapServiceRegistryImpl.
 	 *
@@ -180,6 +184,10 @@ else if ( IntegratorService.class.equals( serviceRole ) ) {
 
 	@Override
 	public void destroy() {
+		if ( !active ) {
+			return;
+		}
+		active = false;
 		destroy( classLoaderServiceBinding );
 		destroy( strategySelectorBinding );
 		destroy( integratorServiceBinding );
@@ -189,6 +197,10 @@ private void destroy(ServiceBinding serviceBinding) {
 		serviceBinding.getLifecycleOwner().stopService( serviceBinding );
 	}
 
+	public boolean isActive() {
+		return active;
+	}
+
 	@Override
 	public ServiceRegistry getParentServiceRegistry() {
 		return null;
@@ -227,4 +239,31 @@ public <R extends Service> void stopService(ServiceBinding<R> binding) {
 		}
 	}
 
+	@Override
+	public void registerChild(ServiceRegistryImplementor child) {
+		if ( childRegistries == null ) {
+			childRegistries = new HashSet<ServiceRegistryImplementor>();
+		}
+		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/...-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java b/...-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java
@@ -35,7 +35,6 @@
 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 <R extends Service> void configureService(ServiceBinding<R> serviceBindin
 			( (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
@@ -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<ServiceBinding> serviceBindingList = CollectionHelper.arrayList( 20 );
 
+	private Set<ServiceRegistryImplementor> childRegistries;
+
 	@SuppressWarnings( {"UnusedDeclaration"})
 	protected AbstractServiceRegistryImpl() {
 		this( (ServiceRegistryImplementor) null );
@@ -78,6 +82,8 @@ protected 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 AbstractServiceRegistryImpl(BootstrapServiceRegistry bootstrapServiceRegi
 		}
 		this.parent = (ServiceRegistryImplementor) bootstrapServiceRegistry;
 		this.allowCrawling = ConfigurationHelper.getBoolean( ALLOW_CRAWLING, Environment.getProperties(), true );
+
+		this.parent.registerChild( this );
 	}
 
 	@SuppressWarnings({ "unchecked" })
@@ -309,18 +317,36 @@ public <R extends Service> void startService(ServiceBinding<R> serviceBinding) {
 		}
 	}
 
+	private boolean active = true;
+
+	public boolean isActive() {
+		return active;
+	}
+
 	@Override
     @SuppressWarnings( {"unchecked"})
 	public void destroy() {
-		synchronized ( serviceBindingList ) {
-			ListIterator<ServiceBinding> serviceBindingsIterator = serviceBindingList.listIterator( serviceBindingList.size() );
-			while ( serviceBindingsIterator.hasPrevious() ) {
-				final ServiceBinding serviceBinding = serviceBindingsIterator.previous();
-				serviceBinding.getLifecycleOwner().stopService( serviceBinding );
+		if ( !active ) {
+			return;
+		}
+
+		active = false;
+		try {
+			synchronized (serviceBindingList) {
+				ListIterator<ServiceBinding> serviceBindingsIterator = serviceBindingList.listIterator(
+						serviceBindingList.size()
+				);
+				while ( serviceBindingsIterator.hasPrevious() ) {
+					final ServiceBinding serviceBinding = serviceBindingsIterator.previous();
+					serviceBinding.getLifecycleOwner().stopService( serviceBinding );
+				}
+				serviceBindingList.clear();
 			}
-			serviceBindingList.clear();
+			serviceBindingMap.clear();
+		}
+		finally {
+			parent.deRegisterChild( this );
 		}
-		serviceBindingMap.clear();
 	}
 
 	@Override
@@ -335,4 +361,32 @@ public <R extends Service> void stopService(ServiceBinding<R> binding) {
 			}
 		}
 	}
+
+	@Override
+	public void registerChild(ServiceRegistryImplementor child) {
+		if ( childRegistries == null ) {
+			childRegistries = new HashSet<ServiceRegistryImplementor>();
+		}
+		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
@@ -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/...-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java b/...-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
@@ -75,6 +75,7 @@
 import org.junit.After;
 import org.junit.Before;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.fail;
 
 /**
@@ -459,10 +460,16 @@ protected void releaseSessionFactory() {
 		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;
 	}