HHH-15541 Possible carrier thread pinning synchronized blocks migrated to ReentrantLock

hibernate-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java

@@ -109,36 +109,56 @@ public class StandardServiceRegistryImpl extends AbstractServiceRegistryImpl imp
 	 * Not intended for general use. We need the ability to stop and "reactivate" a registry to allow
 	 * experimentation with technologies such as GraalVM, Quarkus and Cri-O.
 	 */
-	public synchronized void resetAndReactivate(BootstrapServiceRegistry bootstrapServiceRegistry,
+	public void resetAndReactivate(BootstrapServiceRegistry bootstrapServiceRegistry,
 												List<StandardServiceInitiator<?>> serviceInitiators,
 												List<ProvidedService<?>> providedServices,
 												Map<?, ?> configurationValues) {
-		if ( super.isActive() ) {
+		thisLock.lock();
-			throw new IllegalStateException( "Can't reactivate an active registry" );
+		try {
+			if ( super.isActive() ) {
+				throw new IllegalStateException( "Can't reactivate an active registry" );
+			}
+			super.resetParent( bootstrapServiceRegistry );
+			this.configurationValues = new HashMap( configurationValues );
+			super.reactivate();
+			applyServiceRegistrations( serviceInitiators, providedServices );
+		} finally {
+			thisLock.unlock();
 		}
-		super.resetParent( bootstrapServiceRegistry );
-		this.configurationValues = new HashMap( configurationValues );
-		super.reactivate();
-		applyServiceRegistrations( serviceInitiators, providedServices );
 	}
 
 
 	@Override
-	public synchronized <R extends Service> R initiateService(ServiceInitiator<R> serviceInitiator) {
+	public <R extends Service> R initiateService(ServiceInitiator<R> serviceInitiator) {
-		// todo : add check/error for unexpected initiator types?
+		thisLock.lock();
-		return ( (StandardServiceInitiator<R>) serviceInitiator ).initiateService( configurationValues, this );
+		try {
-	}
+			// todo : add check/error for unexpected initiator types?
-
+			return ( (StandardServiceInitiator<R>) serviceInitiator ).initiateService( configurationValues, this );
-	@Override
+		} finally {
-	public synchronized <R extends Service> void configureService(ServiceBinding<R> serviceBinding) {
+			thisLock.unlock();
-		if ( serviceBinding.getService() instanceof Configurable ) {
-			( (Configurable) serviceBinding.getService() ).configure( configurationValues );
 		}
 	}
 
 	@Override
-	public synchronized void destroy() {
+	public <R extends Service> void configureService(ServiceBinding<R> serviceBinding) {
-		super.destroy();
+		thisLock.lock();
-		this.configurationValues = null;
+		try {
+			if ( serviceBinding.getService() instanceof Configurable ) {
+				( (Configurable) serviceBinding.getService() ).configure( configurationValues );
+			}
+		} finally {
+			thisLock.unlock();
+		}
+	}
+
+	@Override
+	public void destroy() {
+		thisLock.lock();
+		try {
+			super.destroy();
+			this.configurationValues = null;
+		} finally {
+			thisLock.unlock();
+		}
 	}
 }

hibernate-core/src/main/java/org/hibernate/cache/spi/AbstractRegionFactory.java

@@ -9,6 +9,8 @@ package org.hibernate.cache.spi;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.hibernate.boot.spi.SessionFactoryOptions;
 import org.hibernate.cache.CacheException;
@@ -46,6 +48,7 @@ public abstract class AbstractRegionFactory implements RegionFactory {
 
 	private SessionFactoryOptions options;
 
+	protected final Lock thisLock = new ReentrantLock();
 
 	protected boolean isStarted() {
 		if ( started.get() ) {
@@ -83,7 +86,8 @@ public abstract class AbstractRegionFactory implements RegionFactory {
 	@Override
 	public final void start(SessionFactoryOptions settings, Map<String,Object> configValues) throws CacheException {
 		if ( started.compareAndSet( false, true ) ) {
-			synchronized (this) {
+			thisLock.lock();
+			try {
 				this.options = settings;
 				try {
 					prepareForUse( settings, configValues );
@@ -94,6 +98,8 @@ public abstract class AbstractRegionFactory implements RegionFactory {
 					started.set( false );
 					startingException = e;
 				}
+			} finally {
+				thisLock.unlock();
 			}
 		}
 		else {
@@ -106,7 +112,8 @@ public abstract class AbstractRegionFactory implements RegionFactory {
 	@Override
 	public final void stop() {
 		if ( started.compareAndSet( true, false ) ) {
-			synchronized ( this ) {
+			thisLock.lock();
+			try {
 				try {
 					releaseFromUse();
 				}
@@ -114,6 +121,8 @@ public abstract class AbstractRegionFactory implements RegionFactory {
 					options = null;
 					startingException = null;
 				}
+			} finally {
+				thisLock.unlock();
 			}
 		}
 		else {

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

@@ -14,6 +14,8 @@ import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 
 import org.hibernate.boot.registry.BootstrapServiceRegistry;
@@ -72,6 +74,9 @@ public abstract class AbstractServiceRegistryImpl
 
 	private final AtomicBoolean active = new AtomicBoolean( true );
 
+	protected final Lock thisLock = new ReentrantLock();
+	private final Lock serviceBindingListLock = new ReentrantLock();
+
 	protected AbstractServiceRegistryImpl(@Nullable ServiceRegistryImplementor parent) {
 		this( parent, true );
 	}
@@ -199,7 +204,8 @@ public abstract class AbstractServiceRegistryImpl
 		}
 
 		//Any service initialization needs synchronization
-		synchronized ( this ) {
+		thisLock.lock();
+		try {
 			// Check again after having acquired the lock:
 			service = serviceRole.cast( initializedServiceByRole.get( serviceRole ) );
 			if ( service != null ) {
@@ -219,13 +225,18 @@ public abstract class AbstractServiceRegistryImpl
 				initializedServiceByRole.put( serviceRole, service );
 			}
 			return service;
+		} finally {
+			thisLock.unlock();
 		}
 	}
 
 	protected <R extends Service> void registerService(ServiceBinding<R> serviceBinding, R service) {
 		serviceBinding.setService( service );
-		synchronized ( serviceBindingList ) {
+		serviceBindingListLock.lock();
+		try {
 			serviceBindingList.add( serviceBinding );
+		} finally {
+			serviceBindingListLock.unlock();
 		}
 	}
 
@@ -351,78 +362,101 @@ public abstract class AbstractServiceRegistryImpl
 	}
 
 	@Override
-	public synchronized void destroy() {
+	public void destroy() {
-		if ( active.compareAndSet( true, false ) ) {
+		thisLock.lock();
-			try {
+		try {
-				//First thing, make sure that the fast path read is disabled so that
+			if ( active.compareAndSet( true, false ) ) {
-				//threads not owning the synchronization lock can't get an invalid Service:
+				try {
-				initializedServiceByRole.clear();
+					//First thing, make sure that the fast path read is disabled so that
-				synchronized (serviceBindingList) {
+					//threads not owning the synchronization lock can't get an invalid Service:
-					ListIterator<ServiceBinding<?>> serviceBindingsIterator = serviceBindingList.listIterator(
+					initializedServiceByRole.clear();
-							serviceBindingList.size()
+					serviceBindingListLock.lock();
-					);
+					try {
-					while ( serviceBindingsIterator.hasPrevious() ) {
+						ListIterator<ServiceBinding<?>> serviceBindingsIterator = serviceBindingList.listIterator(
-						final ServiceBinding<?> serviceBinding = serviceBindingsIterator.previous();
+								serviceBindingList.size()
-						serviceBinding.getLifecycleOwner().stopService( serviceBinding );
+						);
+						while ( serviceBindingsIterator.hasPrevious() ) {
+							final ServiceBinding<?> serviceBinding = serviceBindingsIterator.previous();
+							serviceBinding.getLifecycleOwner().stopService( serviceBinding );
+						}
+						serviceBindingList.clear();
+					} finally {
+						serviceBindingListLock.unlock();
 					}
-					serviceBindingList.clear();
+					serviceBindingMap.clear();
 				}
-				serviceBindingMap.clear();
+				finally {
-			}
+					if ( parent != null ) {
-			finally {
+						parent.deRegisterChild( this );
-				if ( parent != null ) {
+					}
-					parent.deRegisterChild( this );
 				}
 			}
+		} finally {
+			thisLock.unlock();
 		}
 	}
 
 	@Override
-	public synchronized <R extends Service> void stopService(ServiceBinding<R> binding) {
+	public <R extends Service> void stopService(ServiceBinding<R> binding) {
-		final Service service = binding.getService();
+		thisLock.lock();
-		if ( service instanceof Stoppable ) {
+		try {
-			try {
+			final Service service = binding.getService();
-				( (Stoppable) service ).stop();
+			if ( service instanceof Stoppable ) {
-			}
+				try {
-			catch ( Exception e ) {
+					( (Stoppable) service ).stop();
-				log.unableToStopService( service.getClass(), e );
+				}
+				catch ( Exception e ) {
+					log.unableToStopService( service.getClass(), e );
+				}
 			}
+		} finally {
+			thisLock.unlock();
 		}
 	}
 
 	@Override
-	public synchronized void registerChild(ServiceRegistryImplementor child) {
+	public void registerChild(ServiceRegistryImplementor child) {
-		if ( childRegistries == null ) {
+		thisLock.lock();
-			childRegistries = new HashSet<>();
+		try {
-		}
+			if ( childRegistries == null ) {
-		if ( !childRegistries.add( child ) ) {
+				childRegistries = new HashSet<>();
-			log.warnf(
-					"Child ServiceRegistry [%s] was already registered; this will end badly later...",
-					child
-			);
-		}
-	}
-
-	@Override
-	public synchronized void deRegisterChild(ServiceRegistryImplementor child) {
-		if ( childRegistries == null ) {
-			throw new IllegalStateException( "No child ServiceRegistry registrations found" );
-		}
-		childRegistries.remove( child );
-		if ( childRegistries.isEmpty() ) {
-			if ( autoCloseRegistry ) {
-				log.debug(
-						"Implicitly destroying ServiceRegistry on de-registration " +
-								"of all child ServiceRegistries"
-				);
-				destroy();
 			}
-			else {
+			if ( !childRegistries.add( child ) ) {
-				log.debug(
+				log.warnf(
-						"Skipping implicitly destroying ServiceRegistry on de-registration " +
+						"Child ServiceRegistry [%s] was already registered; this will end badly later...",
-								"of all child ServiceRegistries"
+						child
 				);
 			}
+		} finally {
+			thisLock.unlock();
+		}
+	}
+
+	@Override
+	public void deRegisterChild(ServiceRegistryImplementor child) {
+		thisLock.lock();
+		try {
+			if ( childRegistries == null ) {
+				throw new IllegalStateException( "No child ServiceRegistry registrations found" );
+			}
+			childRegistries.remove( child );
+			if ( childRegistries.isEmpty() ) {
+				if ( autoCloseRegistry ) {
+					log.debug(
+							"Implicitly destroying ServiceRegistry on de-registration " +
+									"of all child ServiceRegistries"
+					);
+					destroy();
+				}
+				else {
+					log.debug(
+							"Skipping implicitly destroying ServiceRegistry on de-registration " +
+									"of all child ServiceRegistries"
+					);
+				}
+			}
+		} finally {
+			thisLock.unlock();
 		}
 	}
 
@@ -430,19 +464,24 @@ public abstract class AbstractServiceRegistryImpl
 	 * Not intended for general use. We need the ability to stop and "reactivate" a registry to allow
 	 * experimentation with technologies such as GraalVM, Quarkus and Cri-O.
 	 */
-	public synchronized void resetParent(@Nullable BootstrapServiceRegistry newParent) {
+	public void resetParent(@Nullable BootstrapServiceRegistry newParent) {
-		if ( this.parent != null ) {
+		thisLock.lock();
-			this.parent.deRegisterChild( this );
+		try {
-		}
+			if ( this.parent != null ) {
-		if ( newParent != null ) {
+				this.parent.deRegisterChild( this );
-			if ( !(newParent instanceof ServiceRegistryImplementor) ) {
-				throw new IllegalArgumentException( "ServiceRegistry parent needs to implement ServiceRegistryImplementor" );
 			}
-			this.parent = (ServiceRegistryImplementor) newParent;
+			if ( newParent != null ) {
-			this.parent.registerChild( this );
+				if ( !(newParent instanceof ServiceRegistryImplementor) ) {
-		}
+					throw new IllegalArgumentException( "ServiceRegistry parent needs to implement ServiceRegistryImplementor" );
-		else {
+				}
-			this.parent = null;
+				this.parent = (ServiceRegistryImplementor) newParent;
+				this.parent.registerChild( this );
+			}
+			else {
+				this.parent = null;
+			}
+		} finally {
+			thisLock.unlock();
 		}
 	}
 
@@ -477,9 +516,14 @@ public abstract class AbstractServiceRegistryImpl
 	 * Not intended for general use. We need the ability to stop and "reactivate" a registry to allow
 	 * experimentation with technologies such as GraalVM, Quarkus and Cri-O.
 	 */
-	public synchronized void reactivate() {
+	public void reactivate() {
-		if ( !active.compareAndSet( false, true ) ) {
+		thisLock.lock();
-			throw new IllegalStateException( "Was not inactive, could not reactivate" );
+		try {
+			if ( !active.compareAndSet( false, true ) ) {
+				throw new IllegalStateException( "Was not inactive, could not reactivate" );
+			}
+		} finally {
+			thisLock.unlock();
 		}
 	}