From 7e4e4785055186b780da9641bb655b6651949e27 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Wed, 13 Dec 2023 11:37:20 +0100 Subject: [PATCH] HHH-17489 Fix concurrent modifications during post load processing --- .../internal/StatefulPersistenceContext.java | 106 +++++++++--------- ...luesSourceProcessingStateStandardImpl.java | 28 +++++ .../spi/JdbcValuesSourceProcessingState.java | 12 +- .../orm/test/jpa/collection/PostLoadTest.java | 73 ++++++++---- 4 files changed, 140 insertions(+), 79 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java b/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java index 0ed803d56c..ca96d0d891 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java @@ -407,7 +407,7 @@ public class StatefulPersistenceContext implements PersistenceContext { } assert holder.entityInitializer == null || holder.entityInitializer == initializer; holder.entityInitializer = initializer; - holder.processingState = processingState; + processingState.registerLoadingEntityHolder( holder ); return holder; } @@ -423,57 +423,59 @@ public class StatefulPersistenceContext implements PersistenceContext { @Override public void postLoad(JdbcValuesSourceProcessingState processingState, Consumer holderConsumer) { - if ( entitiesByKey == null ) { + final Callback callback = processingState.getExecutionContext().getCallback(); + if ( processingState.getLoadingEntityHolders() != null ) { + final EventListenerGroup listenerGroup = getSession().getFactory() + .getFastSessionServices() + .eventListenerGroup_POST_LOAD; + final PostLoadEvent postLoadEvent = processingState.getPostLoadEvent(); + for ( final EntityHolder holder : processingState.getLoadingEntityHolders() ) { + processLoadedEntityHolder( holder, listenerGroup, postLoadEvent, callback, holderConsumer ); + } + processingState.getLoadingEntityHolders().clear(); + } + if ( processingState.getReloadedEntityHolders() != null ) { + for ( final EntityHolder holder : processingState.getReloadedEntityHolders() ) { + processLoadedEntityHolder( holder, null, null, callback, holderConsumer ); + } + processingState.getReloadedEntityHolders().clear(); + } + } + + private void processLoadedEntityHolder( + EntityHolder holder, + EventListenerGroup listenerGroup, + PostLoadEvent postLoadEvent, + Callback callback, + Consumer holderConsumer) { + if ( holderConsumer != null ) { + holderConsumer.accept( holder ); + } + if ( holder.getEntity() == null ) { + // It's possible that we tried to load an entity and found out it doesn't exist, + // in which case we added an entry with a null proxy and entity. + // Remove that empty entry on post load to avoid unwanted side effects + entitiesByKey.remove( holder.getEntityKey() ); return; } - final Callback callback = processingState.getExecutionContext().getCallback(); - final EventListenerGroup listenerGroup = getSession().getFactory() - .getFastSessionServices() - .eventListenerGroup_POST_LOAD; - final PostLoadEvent postLoadEvent = getSession().isEventSource() ? new PostLoadEvent( getSession().asEventSource() ) : null; - for ( Iterator iterator = entitiesByKey.values().iterator(); iterator.hasNext(); ) { - final EntityHolderImpl holder = iterator.next(); - if ( holder.processingState == processingState ) { - if ( holderConsumer != null ) { - holderConsumer.accept( holder ); - } - if ( holder.entity == null ) { - // It's possible that we tried to load an entity and found out it doesn't exist, - // in which case we added an entry with a null proxy and entity. - // Remove that empty entry on post load to avoid unwanted side effects - iterator.remove(); - continue; - } - if ( postLoadEvent != null ) { - postLoadEvent.reset(); - postLoadEvent.setEntity( holder.entity ) - .setId( holder.entityKey.getIdentifier() ) - .setPersister( holder.getDescriptor() ); - listenerGroup.fireEventOnEachListener( - postLoadEvent, - PostLoadEventListener::onPostLoad - ); - } - - if ( callback != null ) { - callback.invokeAfterLoadActions( - holder.getEntity(), - holder.getDescriptor(), - getSession() - ); - if ( holder.reloaded ) { - callback.invokeAfterLoadActions( - holder.getEntity(), - holder.getDescriptor(), - getSession() - ); - } - } - holder.processingState = null; - holder.entityInitializer = null; - holder.reloaded = false; - } + if ( postLoadEvent != null ) { + postLoadEvent.reset(); + postLoadEvent.setEntity( holder.getEntity() ) + .setId( holder.getEntityKey().getIdentifier() ) + .setPersister( holder.getDescriptor() ); + listenerGroup.fireEventOnEachListener( + postLoadEvent, + PostLoadEventListener::onPostLoad + ); } + if ( callback != null ) { + callback.invokeAfterLoadActions( + holder.getEntity(), + holder.getDescriptor(), + getSession() + ); + } + ( (EntityHolderImpl) holder ).entityInitializer = null; } @Override @@ -2133,8 +2135,6 @@ public class StatefulPersistenceContext implements PersistenceContext { Object entity; Object proxy; EntityInitializer entityInitializer; - JdbcValuesSourceProcessingState processingState; - boolean reloaded; EntityHolderState state; private EntityHolderImpl(EntityKey entityKey, EntityPersister descriptor, Object entity, Object proxy) { @@ -2173,9 +2173,7 @@ public class StatefulPersistenceContext implements PersistenceContext { @Override public void markAsReloaded(JdbcValuesSourceProcessingState processingState) { - assert this.processingState == null; - this.reloaded = true; - this.processingState = processingState; + processingState.registerReloadedEntityHolder( this ); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/internal/JdbcValuesSourceProcessingStateStandardImpl.java b/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/internal/JdbcValuesSourceProcessingStateStandardImpl.java index a4ea7c9dcb..e589876b3b 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/internal/JdbcValuesSourceProcessingStateStandardImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/internal/JdbcValuesSourceProcessingStateStandardImpl.java @@ -40,6 +40,8 @@ public class JdbcValuesSourceProcessingStateStandardImpl implements JdbcValuesSo private final ExecutionContext executionContext; private final JdbcValuesSourceProcessingOptions processingOptions; + private List loadingEntityHolders; + private List reloadedEntityHolders; private Map initializerByUniquKeyMap; private Map loadingCollectionMap; private List arrayInitializers; @@ -89,6 +91,32 @@ public class JdbcValuesSourceProcessingStateStandardImpl implements JdbcValuesSo return postLoadEvent; } + @Override + public void registerLoadingEntityHolder(EntityHolder holder) { + if ( loadingEntityHolders == null ) { + loadingEntityHolders = new ArrayList<>(); + } + loadingEntityHolders.add( holder ); + } + + @Override + public List getLoadingEntityHolders() { + return loadingEntityHolders; + } + + @Override + public void registerReloadedEntityHolder(EntityHolder holder) { + if ( reloadedEntityHolders == null ) { + reloadedEntityHolders = new ArrayList<>(); + } + reloadedEntityHolders.add( holder ); + } + + @Override + public List getReloadedEntityHolders() { + return reloadedEntityHolders; + } + @Override public void registerInitializer(EntityUniqueKey entityKey, Initializer initializer) { if ( initializerByUniquKeyMap == null ) { diff --git a/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/spi/JdbcValuesSourceProcessingState.java b/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/spi/JdbcValuesSourceProcessingState.java index de53bb6573..57cf751422 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/spi/JdbcValuesSourceProcessingState.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/results/jdbc/spi/JdbcValuesSourceProcessingState.java @@ -6,7 +6,10 @@ */ package org.hibernate.sql.results.jdbc.spi; +import java.util.List; + import org.hibernate.engine.spi.CollectionKey; +import org.hibernate.engine.spi.EntityHolder; import org.hibernate.engine.spi.EntityUniqueKey; import org.hibernate.engine.spi.SharedSessionContractImplementor; import org.hibernate.event.spi.PostLoadEvent; @@ -39,13 +42,20 @@ public interface JdbcValuesSourceProcessingState { PreLoadEvent getPreLoadEvent(); PostLoadEvent getPostLoadEvent(); + void registerLoadingEntityHolder(EntityHolder holder); + + List getLoadingEntityHolders(); + + void registerReloadedEntityHolder(EntityHolder holder); + + List getReloadedEntityHolders(); + void registerInitializer( EntityUniqueKey entityKey, Initializer initializer); Initializer findInitializer(EntityUniqueKey entityKey); - /** * Find a LoadingCollectionEntry locally to this context. * diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/collection/PostLoadTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/collection/PostLoadTest.java index ec87c5b49d..cc591e4bec 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/collection/PostLoadTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/collection/PostLoadTest.java @@ -6,44 +6,57 @@ */ package org.hibernate.orm.test.jpa.collection; -import java.util.HashSet; -import java.util.Set; +import java.util.List; -import org.hibernate.testing.TestForIssue; import org.hibernate.testing.orm.junit.EntityManagerFactoryScope; +import org.hibernate.testing.orm.junit.Jira; import org.hibernate.testing.orm.junit.Jpa; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import static org.junit.Assert.assertEquals; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Root; -@TestForIssue(jiraKey = "HHH-6043") -@Jpa( - annotatedClasses = { Child.class, Parent.class } -) +import static org.junit.jupiter.api.Assertions.assertEquals; + +@Jpa( annotatedClasses = { Child.class, Parent.class } ) public class PostLoadTest { + @BeforeAll + public void setUp(EntityManagerFactoryScope scope) { + scope.inTransaction( entityManager -> { + final Parent parent1 = new Parent(); + parent1.setId( 1 ); + final Child child1 = new Child(); + child1.setId( 1 ); + child1.setDaddy( parent1 ); + entityManager.persist( parent1 ); + entityManager.persist( child1 ); + final Parent parent2 = new Parent(); + parent2.setId( 2 ); + final Child child2 = new Child(); + child2.setId( 2 ); + child2.setDaddy( parent2 ); + entityManager.persist( parent2 ); + entityManager.persist( child2 ); + } ); + } + + @AfterAll + public void tearDown(EntityManagerFactoryScope scope) { + scope.inTransaction( entityManager -> { + entityManager.createQuery( "delete from Child" ).executeUpdate(); + entityManager.createQuery( "delete from Parent" ).executeUpdate(); + } ); + } /** * Load an entity with a collection of associated entities, that uses a @PostLoad method to * access the association. */ @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-6043" ) public void testAccessAssociatedSetInPostLoad(EntityManagerFactoryScope scope) { - - scope.inTransaction( - entityManager -> { - Child child = new Child(); - child.setId( 1 ); - Parent daddy = new Parent(); - daddy.setId( 1 ); - child.setDaddy( daddy ); - Set children = new HashSet<>(); - children.add( child ); - daddy.setChildren( children ); - - entityManager.persist( daddy ); - } - ); - scope.inEntityManager( entityManager -> { Parent daddy = entityManager.find( Parent.class, 1 ); @@ -52,4 +65,16 @@ public class PostLoadTest { ); } + @Test + @Jira( "https://hibernate.atlassian.net/browse/HHH-17489" ) + public void testAccessAssociatedSetInPostLoadQuery(EntityManagerFactoryScope scope) { + scope.inEntityManager( entityManager -> { + final CriteriaQuery cq = entityManager.getCriteriaBuilder().createQuery( Parent.class ); + final Root from = cq.from( Parent.class ); + final List parents = entityManager.createQuery( cq.select( from ) ).getResultList(); + assertEquals( 2, parents.size() ); + assertEquals( 1, parents.get( 0 ).getNrOfChildren() ); + assertEquals( 1, parents.get( 1 ).getNrOfChildren() ); + } ); + } }