From 0f72aef2e972a30115c9026fe9228a32eaae09ab Mon Sep 17 00:00:00 2001 From: Andrea Boriero Date: Fri, 23 Aug 2024 13:48:34 +0200 Subject: [PATCH 1/2] HHH-18546 Add test for issue --- ...PendingBulkOperationCleanupActionTest.java | 339 ++++++++++++++++++ 1 file changed, 339 insertions(+) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/cache/PendingBulkOperationCleanupActionTest.java diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/cache/PendingBulkOperationCleanupActionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/cache/PendingBulkOperationCleanupActionTest.java new file mode 100644 index 000000000000..38a6b063e272 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/cache/PendingBulkOperationCleanupActionTest.java @@ -0,0 +1,339 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * Copyright Red Hat Inc. and Hibernate Authors + */ +package org.hibernate.orm.test.cache; + +import java.util.Properties; + +import org.hibernate.cache.CacheException; +import org.hibernate.cache.cfg.spi.DomainDataRegionBuildingContext; +import org.hibernate.cache.cfg.spi.DomainDataRegionConfig; +import org.hibernate.cache.cfg.spi.EntityDataCachingConfig; +import org.hibernate.cache.internal.DefaultCacheKeysFactory; +import org.hibernate.cache.spi.CacheKeysFactory; +import org.hibernate.cache.spi.DomainDataRegion; +import org.hibernate.cache.spi.access.AccessType; +import org.hibernate.cache.spi.access.EntityDataAccess; +import org.hibernate.cache.spi.access.SoftLock; +import org.hibernate.cache.spi.support.AbstractEntityDataAccess; +import org.hibernate.cache.spi.support.DomainDataRegionImpl; +import org.hibernate.cache.spi.support.DomainDataStorageAccess; +import org.hibernate.cache.spi.support.RegionFactoryTemplate; +import org.hibernate.cfg.AvailableSettings; +import org.hibernate.engine.spi.SharedSessionContractImplementor; + +import org.hibernate.testing.cache.CachingRegionFactory; +import org.hibernate.testing.cache.MapStorageAccessImpl; +import org.hibernate.testing.orm.junit.DomainModel; +import org.hibernate.testing.orm.junit.JiraKey; +import org.hibernate.testing.orm.junit.ServiceRegistry; +import org.hibernate.testing.orm.junit.SessionFactory; +import org.hibernate.testing.orm.junit.SessionFactoryScope; +import org.hibernate.testing.orm.junit.Setting; +import org.hibernate.testing.orm.junit.SettingProvider; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.jboss.logging.Logger; + +import jakarta.persistence.Cacheable; +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.Table; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +@DomainModel( + annotatedClasses = { + PendingBulkOperationCleanupActionTest.Customer.class + } +) +@SessionFactory() +@ServiceRegistry( + settings = { + @Setting(name = AvailableSettings.USE_SECOND_LEVEL_CACHE, value = "true"), + @Setting(name = AvailableSettings.ALLOW_UPDATE_OUTSIDE_TRANSACTION, value = "true"), + }, + settingProviders = @SettingProvider( + settingName = AvailableSettings.CACHE_REGION_FACTORY, + provider = PendingBulkOperationCleanupActionTest.CacheRegionFactoryProvider.class + ) +) +@JiraKey( "HHH-18546" ) +public class PendingBulkOperationCleanupActionTest { + + private static final TestCachingRegionFactory CACHING_REGION_FACTORY = new TestCachingRegionFactory(); + + public static class CacheRegionFactoryProvider implements SettingProvider.Provider { + @Override + public TestCachingRegionFactory getSetting() { + return CACHING_REGION_FACTORY; + } + } + + @BeforeEach + public void before(SessionFactoryScope scope) { + scope.inTransaction( + session -> + session.persist( new Customer( 1, "Samuel" ) ) + ); + CACHING_REGION_FACTORY.getTestDomainDataRegion().getTestEntityDataAccess().reset(); + } + + @AfterEach + public void after(SessionFactoryScope scope) { + scope.inTransaction( + session -> + session.createQuery( "delete Customer" ).executeUpdate() + ); + CACHING_REGION_FACTORY.getTestDomainDataRegion().getTestEntityDataAccess().reset(); + } + + @Test + public void testUpdateCachedEntity(SessionFactoryScope scope) { + scope.inTransaction( + session -> + session.createNativeQuery( "update Customer set id = id" ).executeUpdate() + ); + assertThat( isLockRegionCalled() ) + .as( "EntityDataAccess lockRegion method has not been called" ) + .isTrue(); + // Region unlock is a BulkOperationCleanUpAction executed after Transaction commit + assertThat( isUnlockRegionCalled() ) + .as( "EntityDataAccess unlockRegion method has not been called" ) + .isTrue(); + } + + @Test + public void testPendingBulkOperationActionsAreExecutedWhenSessionIsClosed(SessionFactoryScope scope) { + scope.inSession( + session -> + session.createNativeQuery( "update Customer set id = id" ).executeUpdate() + ); + + assertThat( isLockRegionCalled() ) + .as( "EntityDataAccess lockRegion method has not been called" ) + .isTrue(); + + // Because the update is executed outside a transaction, region unlock BulkOperationCleanUpAction has not been executed + // and when the session is closed it's a pending action + assertThat( isUnlockRegionCalled() ) + .as( "EntityDataAccess unlockRegion method has not been called" ) + .isTrue(); + } + + private static boolean isUnlockRegionCalled() { + return CACHING_REGION_FACTORY.getTestDomainDataRegion() + .getTestEntityDataAccess() + .isUnlockRegionCalled(); + } + + private static boolean isLockRegionCalled() { + return CACHING_REGION_FACTORY.getTestDomainDataRegion() + .getTestEntityDataAccess() + .isLockRegionCalled(); + } + + @Entity(name = "Customer") + @Table(name = "Customer") + @Cacheable + public static class Customer { + @Id + private int id; + + private String name; + + public Customer() { + } + + public Customer(int id, String name) { + this.id = id; + this.name = name; + } + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } + + public static class TestCachingRegionFactory extends CachingRegionFactory { + private static final Logger LOG = Logger.getLogger( org.hibernate.testing.cache.CachingRegionFactory.class.getName() ); + + public static final String DEFAULT_ACCESSTYPE = "DefaultAccessType"; + private final CacheKeysFactory cacheKeysFactory; + private TestDomainDataRegion testDomainDataRegion; + + public TestCachingRegionFactory() { + this( DefaultCacheKeysFactory.INSTANCE, null ); + } + + public TestCachingRegionFactory(CacheKeysFactory cacheKeysFactory) { + this( cacheKeysFactory, null ); + } + + public TestCachingRegionFactory(Properties properties) { + this( DefaultCacheKeysFactory.INSTANCE, properties ); + } + + public TestCachingRegionFactory(CacheKeysFactory cacheKeysFactory, Properties properties) { + LOG.warn( "org.hibernate.testing.cache.CachingRegionFactory should be only used for testing." ); + this.cacheKeysFactory = cacheKeysFactory; + } + + @Override + public DomainDataRegion buildDomainDataRegion( + DomainDataRegionConfig regionConfig, DomainDataRegionBuildingContext buildingContext) { + if ( testDomainDataRegion == null ) { + testDomainDataRegion = new TestDomainDataRegion( + regionConfig, + this, + new MapStorageAccessImpl(), + cacheKeysFactory, + buildingContext + ); + } + return testDomainDataRegion; + } + + @Override + protected void releaseFromUse() { + testDomainDataRegion = null; + } + + public TestDomainDataRegion getTestDomainDataRegion() { + return testDomainDataRegion; + } + } + + public static class TestDomainDataRegion extends DomainDataRegionImpl { + + TestEntityDataAccess testEntityDataAccess; + + public TestDomainDataRegion( + DomainDataRegionConfig regionConfig, + RegionFactoryTemplate regionFactory, + DomainDataStorageAccess domainDataStorageAccess, + CacheKeysFactory defaultKeysFactory, + DomainDataRegionBuildingContext buildingContext) { + super( regionConfig, regionFactory, domainDataStorageAccess, defaultKeysFactory, buildingContext ); + } + + @Override + public EntityDataAccess generateEntityAccess(EntityDataCachingConfig entityAccessConfig) { + if ( testEntityDataAccess == null ) { + testEntityDataAccess = new TestEntityDataAccess( + this, + getEffectiveKeysFactory(), + getCacheStorageAccess() + ); + } + return testEntityDataAccess; + } + + public TestEntityDataAccess getTestEntityDataAccess() { + return testEntityDataAccess; + } + + @Override + public void destroy() throws CacheException { + testEntityDataAccess = null; + } + } + + public static class TestEntityDataAccess extends AbstractEntityDataAccess { + + private boolean isUnlockRegionCalled = false; + private boolean lockRegionCalled = false; + + public TestEntityDataAccess( + DomainDataRegion region, + CacheKeysFactory cacheKeysFactory, + DomainDataStorageAccess storageAccess) { + super( region, cacheKeysFactory, storageAccess ); + } + + @Override + public boolean insert(SharedSessionContractImplementor session, Object key, Object value, Object version) { + return false; + } + + @Override + public boolean afterInsert(SharedSessionContractImplementor session, Object key, Object value, Object version) { + return false; + } + + @Override + public boolean update( + SharedSessionContractImplementor session, + Object key, + Object value, + Object currentVersion, + Object previousVersion) { + return false; + } + + @Override + public boolean afterUpdate( + SharedSessionContractImplementor session, + Object key, + Object value, + Object currentVersion, + Object previousVersion, + SoftLock lock) { + return false; + } + + @Override + public AccessType getAccessType() { + return null; + } + + @Override + public SoftLock lockRegion() { + lockRegionCalled = true; + return super.lockRegion(); + } + + @Override + public void unlockRegion(SoftLock lock) { + super.unlockRegion( lock ); + isUnlockRegionCalled = true; + } + + @Override + public void destroy() { + super.destroy(); + isUnlockRegionCalled = false; + } + + public boolean isUnlockRegionCalled() { + return isUnlockRegionCalled; + } + + public boolean isLockRegionCalled() { + return lockRegionCalled; + } + + public void reset() { + this.isUnlockRegionCalled = false; + this.lockRegionCalled = false; + } + + } + + +} From 9e4b8da24174e165c0e17e11f08d9ba3aa19173b Mon Sep 17 00:00:00 2001 From: Andrea Boriero Date: Fri, 23 Aug 2024 13:50:49 +0200 Subject: [PATCH 2/2] HHH-18546 Clean up any hanging BulkOperationCleanupAction after-txn callbacks on Session close --- .../internal/BulkOperationCleanupAction.java | 25 +++++++++++++- ...fterTransactionCompletionProcessQueue.java | 34 +++++++++++++++++++ .../TransactionCompletionCallbacksImpl.java | 7 ++++ .../org/hibernate/engine/spi/ActionQueue.java | 9 +++++ ...sactionCompletionCallbacksImplementor.java | 6 ++++ .../org/hibernate/internal/SessionImpl.java | 4 +++ 6 files changed, 84 insertions(+), 1 deletion(-) diff --git a/hibernate-core/src/main/java/org/hibernate/action/internal/BulkOperationCleanupAction.java b/hibernate-core/src/main/java/org/hibernate/action/internal/BulkOperationCleanupAction.java index dd2148b7d076..d65a5241350a 100644 --- a/hibernate-core/src/main/java/org/hibernate/action/internal/BulkOperationCleanupAction.java +++ b/hibernate-core/src/main/java/org/hibernate/action/internal/BulkOperationCleanupAction.java @@ -214,7 +214,30 @@ public BeforeTransactionCompletionProcess getBeforeTransactionCompletionProcess( @Override public AfterTransactionCompletionProcess getAfterTransactionCompletionProcess() { - return (success, session) -> { + return new BulkOperationCleanUpAfterTransactionCompletionProcess( + entityCleanups, + collectionCleanups, + naturalIdCleanups + ); + } + + public static final class BulkOperationCleanUpAfterTransactionCompletionProcess + implements AfterTransactionCompletionProcess { + private final Set entityCleanups; + private final Set collectionCleanups; + private final Set naturalIdCleanups; + + public BulkOperationCleanUpAfterTransactionCompletionProcess( + Set entityCleanups, + Set collectionCleanups, + Set naturalIdCleanups) { + this.entityCleanups = entityCleanups; + this.collectionCleanups = collectionCleanups; + this.naturalIdCleanups = naturalIdCleanups; + } + + @Override + public void doAfterTransactionCompletion(boolean success, SharedSessionContractImplementor session) { for ( var cleanup : entityCleanups ) { cleanup.release(); } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/AfterTransactionCompletionProcessQueue.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/AfterTransactionCompletionProcessQueue.java index c4beb9eca9e7..b910e3ee8ad9 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/internal/AfterTransactionCompletionProcessQueue.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/internal/AfterTransactionCompletionProcessQueue.java @@ -5,6 +5,7 @@ package org.hibernate.engine.internal; import org.hibernate.HibernateException; +import org.hibernate.action.internal.BulkOperationCleanupAction; import org.hibernate.cache.CacheException; import org.hibernate.engine.spi.SessionFactoryImplementor; import org.hibernate.engine.spi.SharedSessionContractImplementor; @@ -59,4 +60,37 @@ public void afterTransactionCompletion(boolean success) { } querySpacesToInvalidate.clear(); } + + public void executePendingBulkOperationCleanUpActions() { + AfterCompletionCallback process; + boolean hasPendingBulkOperationCleanUpActions = false; + while ( ( process = processes.poll() ) != null ) { + if ( process instanceof BulkOperationCleanupAction.BulkOperationCleanUpAfterTransactionCompletionProcess ) { + try { + hasPendingBulkOperationCleanUpActions = true; + process.doAfterTransactionCompletion( true, session ); + } + catch (CacheException ce) { + CORE_LOGGER.unableToReleaseCacheLock( ce ); + // continue loop + } + catch (Exception e) { + throw new HibernateException( + "Unable to perform afterTransactionCompletion callback: " + e.getMessage(), + e + ); + } + } + } + + if ( hasPendingBulkOperationCleanUpActions ) { + if ( session.getFactory().getSessionFactoryOptions().isQueryCacheEnabled() ) { + session.getFactory().getCache().getTimestampsCache().invalidate( + querySpacesToInvalidate.toArray( new String[0] ), + session + ); + } + querySpacesToInvalidate.clear(); + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/TransactionCompletionCallbacksImpl.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/TransactionCompletionCallbacksImpl.java index 4f6d70a44ba5..3c95c1c1fe89 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/internal/TransactionCompletionCallbacksImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/internal/TransactionCompletionCallbacksImpl.java @@ -79,4 +79,11 @@ public TransactionCompletionCallbacksImpl forSharing() { } return this; } + + @Override + public void executePendingBulkOperationCleanUpActions() { + if ( afterTransactionProcesses != null ) { + afterTransactionProcesses.executePendingBulkOperationCleanUpActions(); + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/spi/ActionQueue.java b/hibernate-core/src/main/java/org/hibernate/engine/spi/ActionQueue.java index a74fd8955b29..792497dd1805 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/spi/ActionQueue.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/spi/ActionQueue.java @@ -19,6 +19,7 @@ import org.hibernate.AssertionFailure; import org.hibernate.HibernateException; +import org.hibernate.Internal; import org.hibernate.PropertyValueException; import org.hibernate.TransientPropertyValueException; import org.hibernate.action.internal.AbstractEntityInsertAction; @@ -537,6 +538,14 @@ public void afterTransactionCompletion(boolean success) { } } + @Internal + public void executePendingBulkOperationCleanUpActions() { + if ( !isTransactionCoordinatorShared && transactionCompletionCallbacks != null ) { + // Execute completion actions only in transaction owner (aka parent session). + transactionCompletionCallbacks.executePendingBulkOperationCleanUpActions(); + } + } + /** * Execute any registered {@link BeforeTransactionCompletionProcess} */ diff --git a/hibernate-core/src/main/java/org/hibernate/engine/spi/TransactionCompletionCallbacksImplementor.java b/hibernate-core/src/main/java/org/hibernate/engine/spi/TransactionCompletionCallbacksImplementor.java index 10eb8e98cc29..37d959299006 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/spi/TransactionCompletionCallbacksImplementor.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/spi/TransactionCompletionCallbacksImplementor.java @@ -44,4 +44,10 @@ public interface TransactionCompletionCallbacksImplementor extends TransactionCo * the same transaction coordinator. Returns this instance for convenience/fluency. */ TransactionCompletionCallbacksImplementor forSharing(); + + + /** + * Execute all pending {@link org.hibernate.action.internal.BulkOperationCleanupAction} + */ + void executePendingBulkOperationCleanUpActions(); } diff --git a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java index 7d514add8a18..c48197058fd7 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java @@ -403,6 +403,10 @@ public void closeWithoutOpenChecks() { } } finally { + if ( actionQueue.hasAfterTransactionActions() ) { + SESSION_LOGGER.warn( "Closing session with unprocessed clean up bulk operations, forcing their execution" ); + actionQueue.executePendingBulkOperationCleanUpActions(); + } final var statistics = getSessionFactory().getStatistics(); if ( statistics.isStatisticsEnabled() ) { statistics.closeSession();