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 7da3c358f7..37426a214f 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 @@ -11,6 +11,7 @@ import java.util.HashSet; import java.util.List; import java.util.ListIterator; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import org.hibernate.boot.registry.BootstrapServiceRegistry; import org.hibernate.cfg.Environment; @@ -36,6 +37,7 @@ import org.hibernate.service.spi.Stoppable; * Basic implementation of the ServiceRegistry and ServiceRegistryImplementor contracts * * @author Steve Ebersole + * @author Sanne Grinovero */ public abstract class AbstractServiceRegistryImpl implements ServiceRegistryImplementor, ServiceBinding.ServiceLifecycleOwner { @@ -48,16 +50,24 @@ public abstract class AbstractServiceRegistryImpl private final boolean allowCrawling; private final ConcurrentServiceBinding serviceBindingMap = new ConcurrentServiceBinding(); - private ConcurrentServiceBinding roleXref; + private final ConcurrentServiceBinding roleXref = new ConcurrentServiceBinding(); + // The services stored in initializedServiceByRole are completely initialized + // (i.e., configured, dependencies injected, and started) + private final ConcurrentServiceBinding initializedServiceByRole = new ConcurrentServiceBinding(); // IMPL NOTE : the list used for ordered destruction. Cannot used map above because we need to // iterate it in reverse order which is only available through ListIterator // assume 20 services for initial sizing + // All access guarded by synchronization on the serviceBindingList itself. private final List serviceBindingList = CollectionHelper.arrayList( 20 ); + // Guarded by synchronization on this. private boolean autoCloseRegistry; + // Guarded by synchronization on this. private Set childRegistries; + private final AtomicBoolean active = new AtomicBoolean( true ); + @SuppressWarnings( {"UnusedDeclaration"}) protected AbstractServiceRegistryImpl() { this( (ServiceRegistryImplementor) null ); @@ -143,11 +153,9 @@ public abstract class AbstractServiceRegistryImpl } // look for a previously resolved alternate registration - if ( roleXref != null ) { - final Class alternative = roleXref.get( serviceRole ); - if ( alternative != null ) { - return serviceBindingMap.get( alternative ); - } + final Class alternative = roleXref.get( serviceRole ); + if ( alternative != null ) { + return serviceBindingMap.get( alternative ); } // perform a crawl looking for an alternate registration @@ -171,25 +179,37 @@ public abstract class AbstractServiceRegistryImpl } private void registerAlternate(Class alternate, Class target) { - if ( roleXref == null ) { - roleXref = new ConcurrentServiceBinding(); - } roleXref.put( alternate, target ); } @Override public R getService(Class serviceRole) { - final ServiceBinding serviceBinding = locateServiceBinding( serviceRole ); - if ( serviceBinding == null ) { - throw new UnknownServiceException( serviceRole ); + // TODO: should an exception be thrown if active == false??? + R service = serviceRole.cast( initializedServiceByRole.get( serviceRole ) ); + if ( service != null ) { + return service; } - R service = serviceBinding.getService(); - if ( service == null ) { - service = initializeService( serviceBinding ); - } + //Any service initialization needs synchronization + synchronized ( this ) { + // Check again after having acquired the lock: + service = serviceRole.cast( initializedServiceByRole.get( serviceRole ) ); + if ( service != null ) { + return service; + } - return service; + final ServiceBinding serviceBinding = locateServiceBinding( serviceRole ); + if ( serviceBinding == null ) { + throw new UnknownServiceException( serviceRole ); + } + service = serviceBinding.getService(); + if ( service == null ) { + service = initializeService( serviceBinding ); + } + // add the service only after it is completely initialized + initializedServiceByRole.put( serviceRole, service ); + return service; + } } protected void registerService(ServiceBinding serviceBinding, R service) { @@ -320,35 +340,33 @@ public abstract class AbstractServiceRegistryImpl } } - private boolean active = true; - public boolean isActive() { - return active; + return active.get(); } @Override @SuppressWarnings( {"unchecked"}) - public void destroy() { - 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 ); + public synchronized void destroy() { + if ( active.compareAndSet( true, false ) ) { + try { + //First thing, make sure that the fast path read is disabled so that + //threads not owning the synchronization lock can't get an invalid Service: + initializedServiceByRole.clear(); + synchronized (serviceBindingList) { + ListIterator 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(); - } - finally { - parent.deRegisterChild( this ); } } @@ -366,7 +384,7 @@ public abstract class AbstractServiceRegistryImpl } @Override - public void registerChild(ServiceRegistryImplementor child) { + public synchronized void registerChild(ServiceRegistryImplementor child) { if ( childRegistries == null ) { childRegistries = new HashSet(); } @@ -379,7 +397,7 @@ public abstract class AbstractServiceRegistryImpl } @Override - public void deRegisterChild(ServiceRegistryImplementor child) { + public synchronized void deRegisterChild(ServiceRegistryImplementor child) { if ( childRegistries == null ) { throw new IllegalStateException( "No child ServiceRegistry registrations found" ); } diff --git a/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceBinding.java b/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceBinding.java index 7ea954a689..f3a741dff6 100644 --- a/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceBinding.java +++ b/hibernate-core/src/main/java/org/hibernate/service/spi/ServiceBinding.java @@ -31,7 +31,7 @@ public final class ServiceBinding { private final ServiceLifecycleOwner lifecycleOwner; private final Class serviceRole; private final ServiceInitiator serviceInitiator; - private R service; + private volatile R service; public ServiceBinding(ServiceLifecycleOwner lifecycleOwner, Class serviceRole, R service) { this.lifecycleOwner = lifecycleOwner; diff --git a/hibernate-core/src/test/java/org/hibernate/service/ServiceRegistryTest.java b/hibernate-core/src/test/java/org/hibernate/service/ServiceRegistryTest.java new file mode 100644 index 0000000000..8739b28501 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/service/ServiceRegistryTest.java @@ -0,0 +1,157 @@ +package org.hibernate.service; + +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.FutureTask; + +import org.hibernate.boot.registry.StandardServiceInitiator; +import org.hibernate.boot.registry.StandardServiceRegistryBuilder; +import org.hibernate.service.spi.Configurable; +import org.hibernate.service.spi.ServiceRegistryAwareService; +import org.hibernate.service.spi.ServiceRegistryImplementor; +import org.hibernate.service.spi.Startable; + +import org.junit.Test; + +import org.hibernate.testing.TestForIssue; + +import static org.junit.Assert.assertTrue; + +@TestForIssue(jiraKey = "HHH-10427") +public class ServiceRegistryTest { + private final ServiceRegistry registry = buildRegistry(); + private final static int NUMBER_OF_THREADS = 100; + private StandardServiceRegistryBuilder standardServiceRegistryBuilder; + + @Test + public void testOnlyOneInstanceOfTheServiceShouldBeCreated() throws InterruptedException, ExecutionException { + + Future[] serviceIdentities = execute(); + + SlowInitializationService previousResult = null; + for ( Future future : serviceIdentities ) { + final SlowInitializationService result = future.get(); + if ( previousResult == null ) { + previousResult = result; + } + else { + assertTrue( "There are more than one instance of the service", result == previousResult ); + } + + } + + standardServiceRegistryBuilder.destroy( registry ); + + } + + private ServiceRegistry buildRegistry() { + standardServiceRegistryBuilder = new StandardServiceRegistryBuilder(); + return standardServiceRegistryBuilder.addInitiator( new SlowServiceInitiator() ).build(); + } + + private FutureTask[] execute() + throws InterruptedException, ExecutionException { + FutureTask[] results = new FutureTask[NUMBER_OF_THREADS]; + ExecutorService executor = Executors.newFixedThreadPool( NUMBER_OF_THREADS ); + for ( int i = 0; i < NUMBER_OF_THREADS; i++ ) { + results[i] = new FutureTask<>( new ServiceCallable( registry ) ); + executor.execute( results[i] ); + } + return results; + } + + public class ServiceCallable implements Callable { + private final ServiceRegistry registry; + + public ServiceCallable(ServiceRegistry registry) { + this.registry = registry; + } + + @Override + public SlowInitializationService call() throws Exception { + final SlowInitializationService service = registry.getService( SlowInitializationService.class ); + assertTrue( "The service is not initialized", service.isInitialized() ); + assertTrue( "The service is not configured", service.isConfigured() ); + assertTrue( "The service is not started", service.isStarted() ); + return service; + } + } + + public class SlowInitializationService implements ServiceRegistryAwareService, Configurable, Startable, Service { + private final static int TIME_TO_SLEEP = 100; + private boolean initialized; + private boolean configured; + private boolean started; + + public SlowInitializationService() { + try { + Thread.sleep( TIME_TO_SLEEP ); + } + catch (InterruptedException e) { + } + } + + @Override + public void injectServices(ServiceRegistryImplementor serviceRegistry) { + try { + Thread.sleep( TIME_TO_SLEEP ); + } + catch (InterruptedException e) { + } + + initialized = true; + } + + @Override + public void configure(Map configurationValues) { + try { + Thread.sleep( TIME_TO_SLEEP ); + } + catch (InterruptedException e) { + } + + configured = true; + } + + @Override + public void start() { + try { + Thread.sleep( TIME_TO_SLEEP ); + } + catch (InterruptedException e) { + } + + started = true; + } + + public boolean isInitialized() { + return initialized; + } + + public boolean isConfigured() { + return configured; + } + + public boolean isStarted() { + return started; + } + } + + public class SlowServiceInitiator implements StandardServiceInitiator { + + @Override + public Class getServiceInitiated() { + return SlowInitializationService.class; + } + + @Override + public SlowInitializationService initiateService(Map configurationValues, ServiceRegistryImplementor registry) { + return new SlowInitializationService(); + } + } + +}