Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 36 additions & 16 deletions ...-core/src/main/java/org/hibernate/boot/registry/internal/StandardServiceRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,37 +108,57 @@ protected void applyServiceRegistrations(List<StandardServiceInitiator<?>> servi
* 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() ) {
throw new IllegalStateException( "Can't reactivate an active registry" );
thisLock.lock();
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) {
// todo : add check/error for unexpected initiator types?
return ( (StandardServiceInitiator<R>) serviceInitiator ).initiateService( configurationValues, this );
public <R extends Service> R initiateService(ServiceInitiator<R> serviceInitiator) {
thisLock.lock();
try {
// todo : add check/error for unexpected initiator types?
return ( (StandardServiceInitiator<R>) serviceInitiator ).initiateService( configurationValues, this );
} finally {
thisLock.unlock();
}
}

@Override
public synchronized <R extends Service> void configureService(ServiceBinding<R> serviceBinding) {
if ( serviceBinding.getService() instanceof Configurable ) {
( (Configurable) serviceBinding.getService() ).configure( configurationValues );
public <R extends Service> void configureService(ServiceBinding<R> serviceBinding) {
thisLock.lock();
try {
if ( serviceBinding.getService() instanceof Configurable ) {
( (Configurable) serviceBinding.getService() ).configure( configurationValues );
}
} finally {
thisLock.unlock();
}
}

@Override
public synchronized void destroy() {
super.destroy();
this.configurationValues = null;
public void destroy() {
thisLock.lock();
try {
super.destroy();
this.configurationValues = null;
} finally {
thisLock.unlock();
}
}

private static Map<String, Object> normalize(Map<String, Object> configurationValues) {
Expand Down
13 changes: 11 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/cache/spi/AbstractRegionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
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;
Expand Down Expand Up @@ -44,6 +46,7 @@ public abstract class AbstractRegionFactory implements RegionFactory {

private SessionFactoryOptions options;

protected final Lock thisLock = new ReentrantLock();

protected boolean isStarted() {
if ( started.get() ) {
Expand Down Expand Up @@ -81,7 +84,8 @@ protected SessionFactoryOptions getOptions() {
@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 );
Expand All @@ -92,6 +96,8 @@ public final void start(SessionFactoryOptions settings, Map<String,Object> confi
started.set( false );
startingException = e;
}
} finally {
thisLock.unlock();
}
}
else {
Expand All @@ -104,14 +110,17 @@ public final void start(SessionFactoryOptions settings, Map<String,Object> confi
@Override
public final void stop() {
if ( started.compareAndSet( true, false ) ) {
synchronized ( this ) {
thisLock.lock();
try {
try {
releaseFromUse();
}
finally {
options = null;
startingException = null;
}
} finally {
thisLock.unlock();
}
}
else {
Expand Down
184 changes: 114 additions & 70 deletions hibernate-core/src/main/java/org/hibernate/service/internal/AbstractServiceRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
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;
Expand Down Expand Up @@ -70,6 +72,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 );
}
Expand Down Expand Up @@ -197,7 +202,8 @@ private void registerAlternate(Class<?> alternate, Class<?> target) {
}

//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 ) {
Expand All @@ -217,13 +223,18 @@ private void registerAlternate(Class<?> alternate, Class<?> target) {
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();
}
}

Expand Down Expand Up @@ -349,98 +360,126 @@ public boolean isActive() {
}

@Override
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<ServiceBinding<?>> serviceBindingsIterator = serviceBindingList.listIterator(
serviceBindingList.size()
);
while ( serviceBindingsIterator.hasPrevious() ) {
final ServiceBinding<?> serviceBinding = serviceBindingsIterator.previous();
serviceBinding.getLifecycleOwner().stopService( serviceBinding );
public void destroy() {
thisLock.lock();
try {
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();
serviceBindingListLock.lock();
try {
ListIterator<ServiceBinding<?>> serviceBindingsIterator = serviceBindingList.listIterator(
serviceBindingList.size()
);
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 ) {
parent.deRegisterChild( this );
finally {
if ( parent != null ) {
parent.deRegisterChild( this );
}
}
}
} finally {
thisLock.unlock();
}
}

@Override
public synchronized <R extends Service> void stopService(ServiceBinding<R> binding) {
final Service service = binding.getService();
if ( service instanceof Stoppable ) {
try {
( (Stoppable) service ).stop();
}
catch ( Exception e ) {
log.unableToStopService( service.getClass(), e );
public <R extends Service> void stopService(ServiceBinding<R> binding) {
thisLock.lock();
try {
final Service service = binding.getService();
if ( service instanceof Stoppable ) {
try {
( (Stoppable) service ).stop();
}
catch ( Exception e ) {
log.unableToStopService( service.getClass(), e );
}
}
} finally {
thisLock.unlock();
}
}

@Override
public synchronized 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
);
public void registerChild(ServiceRegistryImplementor child) {
thisLock.lock();
try {
if ( childRegistries == null ) {
childRegistries = new HashSet<>();
}
if ( !childRegistries.add( child ) ) {
log.warnf(
"Child ServiceRegistry [%s] was already registered; this will end badly later...",
child
);
}
} finally {
thisLock.unlock();
}
}

@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();
public void deRegisterChild(ServiceRegistryImplementor child) {
thisLock.lock();
try {
if ( childRegistries == null ) {
throw new IllegalStateException( "No child ServiceRegistry registrations found" );
}
else {
log.debug(
"Skipping implicitly destroying ServiceRegistry on de-registration " +
"of all child ServiceRegistries"
);
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();
}
}

/**
* 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) {
if ( this.parent != null ) {
this.parent.deRegisterChild( this );
}
if ( newParent != null ) {
if ( !(newParent instanceof ServiceRegistryImplementor) ) {
throw new IllegalArgumentException( "ServiceRegistry parent needs to implement ServiceRegistryImplementor" );
public void resetParent(@Nullable BootstrapServiceRegistry newParent) {
thisLock.lock();
try {
if ( this.parent != null ) {
this.parent.deRegisterChild( this );
}
this.parent = (ServiceRegistryImplementor) newParent;
this.parent.registerChild( this );
}
else {
this.parent = null;
if ( newParent != null ) {
if ( !(newParent instanceof ServiceRegistryImplementor) ) {
throw new IllegalArgumentException( "ServiceRegistry parent needs to implement ServiceRegistryImplementor" );
}
this.parent = (ServiceRegistryImplementor) newParent;
this.parent.registerChild( this );
}
else {
this.parent = null;
}
} finally {
thisLock.unlock();
}
}

Expand Down Expand Up @@ -475,9 +514,14 @@ public synchronized void resetParent(@Nullable BootstrapServiceRegistry newParen
* 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() {
if ( !active.compareAndSet( false, true ) ) {
throw new IllegalStateException( "Was not inactive, could not reactivate" );
public void reactivate() {
thisLock.lock();
try {
if ( !active.compareAndSet( false, true ) ) {
throw new IllegalStateException( "Was not inactive, could not reactivate" );
}
} finally {
thisLock.unlock();
}
}

Expand Down
Loading