HHH-8923 - Reconsider closing of ServiceRegistry instances

Conflicts:
	hibernate-core/src/main/java/org/hibernate/boot/registry/internal/BootstrapServiceRegistryImpl.java
	hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java
	hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java
	hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java

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.internal.CoreMessageLogger;
 import org.hibernate.internal.util.collections.CollectionHelper;
@@ -65,6 +67,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 );
@@ -72,6 +76,7 @@ public abstract class AbstractServiceRegistryImpl
 
 	protected AbstractServiceRegistryImpl(ServiceRegistryImplementor parent) {
 		this.parent = parent;
+		this.parent.registerChild( this );
 	}
 
 	public AbstractServiceRegistryImpl(BootstrapServiceRegistry bootstrapServiceRegistry) {
@@ -79,6 +84,7 @@ public abstract class AbstractServiceRegistryImpl
 			throw new IllegalArgumentException( "Boot-strap registry was not " );
 		}
 		this.parent = (ServiceRegistryImplementor) bootstrapServiceRegistry;
+		this.parent.registerChild( this );
 	}
 
 	@SuppressWarnings({ "unchecked" })
@@ -261,18 +267,36 @@ public abstract class AbstractServiceRegistryImpl
 		}
 	}
 
+	private boolean active = true;
+
+	public boolean isActive() {
+		return active;
+	}
+
 	@Override
     @SuppressWarnings( {"unchecked"})
 	public void destroy() {
-		synchronized ( serviceBindingList ) {
+		if ( !active ) {
-			ListIterator<ServiceBinding> serviceBindingsIterator = serviceBindingList.listIterator( serviceBindingList.size() );
+			return;
-			while ( serviceBindingsIterator.hasPrevious() ) {
+		}
-				final ServiceBinding serviceBinding = serviceBindingsIterator.previous();
+
-				serviceBinding.getLifecycleOwner().stopService( serviceBinding );
+		active = false;
-			}
+		try {
-			serviceBindingList.clear();
+			synchronized (serviceBindingList) {
+				ListIterator<ServiceBinding> 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
@@ -287,4 +311,32 @@ public abstract class AbstractServiceRegistryImpl
 			}
 		}
 	}
+
+	@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();
+		}
+	}
 }

hibernate-core/src/main/java/org/hibernate/service/internal/BootstrapServiceRegistryImpl.java

@@ -23,7 +23,9 @@
  */
 package org.hibernate.service.internal;
 
+import java.util.HashSet;
 import java.util.LinkedHashSet;
+import java.util.Set;
 
 import org.hibernate.integrator.internal.IntegratorServiceImpl;
 import org.hibernate.integrator.spi.Integrator;
@@ -58,9 +60,13 @@ public class BootstrapServiceRegistryImpl
 	);
 	
 	private static final LinkedHashSet<Integrator> NO_INTEGRATORS = new LinkedHashSet<Integrator>();
+	
+	private boolean active = true;
 
 	private final ServiceBinding<ClassLoaderService> classLoaderServiceBinding;
 	private final ServiceBinding<IntegratorService> integratorServiceBinding;
+	
+	private Set<ServiceRegistryImplementor> childRegistries;
 
 	public BootstrapServiceRegistryImpl() {
 		this( new ClassLoaderServiceImpl(), NO_INTEGRATORS );
@@ -112,10 +118,18 @@ public class BootstrapServiceRegistryImpl
 
 	@Override
 	public void destroy() {
+		if ( !active ) {
+			return;
+		}
+		active = false;
 		destroy( classLoaderServiceBinding );
 		destroy( integratorServiceBinding );
 	}
 	
+	public boolean isActive() {
+		return active;
+	}
+	
 	private void destroy(ServiceBinding serviceBinding) {
 		serviceBinding.getLifecycleOwner().stopService( serviceBinding );
 	}
@@ -158,4 +172,26 @@ public class BootstrapServiceRegistryImpl
 		}
 	}
 
+	@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();
+		}
+	}
+
 }

hibernate-core/src/main/java/org/hibernate/service/internal/StandardServiceRegistryImpl.java

@@ -33,7 +33,6 @@ import org.hibernate.service.spi.BasicServiceInitiator;
 import org.hibernate.service.spi.Configurable;
 import org.hibernate.service.spi.ServiceBinding;
 import org.hibernate.service.spi.ServiceInitiator;
-import org.hibernate.service.spi.ServiceRegistryImplementor;
 
 /**
  * Hibernate implementation of the standard service registry.
@@ -76,13 +75,4 @@ public class StandardServiceRegistryImpl extends AbstractServiceRegistryImpl imp
 			( (Configurable) serviceBinding.getService() ).configure( configurationValues );
 		}
 	}
-
-	@Override
-	public void destroy() {
-		super.destroy();
-		if ( getParentServiceRegistry() != null ) {
-			( (ServiceRegistryImplementor) getParentServiceRegistry() ).destroy();
-		}
-	}
-
 }

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);
 }

hibernate-core/src/test/java/org/hibernate/service/internal/ServiceRegistryClosingCascadeTest.java

@@ -0,0 +1,50 @@
+/*
+ * 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 static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.hibernate.SessionFactory;
+import org.hibernate.metamodel.MetadataSources;
+import org.hibernate.service.BootstrapServiceRegistry;
+import org.hibernate.service.BootstrapServiceRegistryBuilder;
+import org.hibernate.testing.junit4.BaseUnitTestCase;
+import org.junit.Test;
+
+/**
+ * @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() );
+	}
+}

hibernate-testing/src/main/java/org/hibernate/testing/junit4/BaseCoreFunctionalTestCase.java

@@ -71,6 +71,7 @@ import org.hibernate.testing.OnFailure;
 import org.hibernate.testing.SkipLog;
 import org.hibernate.testing.cache.CachingRegionFactory;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.fail;
 
 /**
@@ -379,11 +380,17 @@ public abstract class BaseCoreFunctionalTestCase extends BaseUnitTestCase {
 		}
 		sessionFactory.close();
 		sessionFactory = null;
-        if(serviceRegistry == null){
+        if ( serviceRegistry != null ) {
-            return;
+			if ( serviceRegistry.isActive() ) {
-        }
+				try {
-        serviceRegistry.destroy();
+					serviceRegistry.destroy();
-        serviceRegistry=null;
+				}
+				catch (Exception ignore) {
+				}
+				fail( "StandardServiceRegistry was not closed down as expected" );
+			}
+		}
+		serviceRegistry = null;
 	}
 
 	@OnFailure