diff --git a/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java b/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java index 622b1f620f..f1a79d0310 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java @@ -76,6 +76,7 @@ import org.hibernate.query.sqm.tree.SqmStatement; import org.hibernate.query.sqm.tree.delete.SqmDeleteStatement; import org.hibernate.query.sqm.tree.update.SqmUpdateStatement; import org.hibernate.resource.jdbc.spi.JdbcSessionContext; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; import org.hibernate.resource.jdbc.spi.StatementInspector; import org.hibernate.resource.transaction.backend.jta.internal.JtaTransactionCoordinatorImpl; import org.hibernate.resource.transaction.spi.TransactionCoordinator; @@ -124,6 +125,7 @@ public abstract class AbstractSharedSessionContract implements SharedSessionCont private FlushMode flushMode; private boolean autoJoinTransactions; + private final PhysicalConnectionHandlingMode connectionHandlingMode; private CacheMode cacheMode; @@ -169,11 +171,9 @@ public abstract class AbstractSharedSessionContract implements SharedSessionCont sessionEventsManager = new SessionEventListenerManagerImpl( customSessionEventListener.toArray( new SessionEventListener[0] ) ); } - final StatementInspector statementInspector = interpret( options.getStatementInspector() ); - this.jdbcSessionContext = new JdbcSessionContextImpl( this, statementInspector, fastSessionServices ); - this.entityNameResolver = new CoordinatingEntityNameResolver( factory, interceptor ); + final StatementInspector statementInspector = interpret( options.getStatementInspector() ); if ( options instanceof SharedSessionCreationOptions && ( (SharedSessionCreationOptions) options ).isTransactionCoordinatorShared() ) { if ( options.getConnection() != null ) { throw new SessionException( "Cannot simultaneously share transaction context and specify connection" ); @@ -195,18 +195,27 @@ public abstract class AbstractSharedSessionContract implements SharedSessionCont ); autoJoinTransactions = false; } - if ( sharedOptions.getPhysicalConnectionHandlingMode() != this.jdbcCoordinator.getLogicalConnection().getConnectionHandlingMode() ) { + this.connectionHandlingMode = this.jdbcCoordinator.getLogicalConnection().getConnectionHandlingMode(); + if ( sharedOptions.getPhysicalConnectionHandlingMode() != this.connectionHandlingMode ) { log.debug( "Session creation specified 'PhysicalConnectionHandlingMode' which is invalid in conjunction " + "with sharing JDBC connection between sessions; ignoring" ); } + this.jdbcSessionContext = new JdbcSessionContextImpl( this, statementInspector, + connectionHandlingMode, fastSessionServices ); + addSharedSessionTransactionObserver( transactionCoordinator ); } else { this.isTransactionCoordinatorShared = false; this.autoJoinTransactions = options.shouldAutoJoinTransactions(); + this.connectionHandlingMode = options.getPhysicalConnectionHandlingMode(); + this.jdbcSessionContext = new JdbcSessionContextImpl( this, statementInspector, + connectionHandlingMode, fastSessionServices ); + // This must happen *after* the JdbcSessionContext was initialized, + // because some of the calls below retrieve this context indirectly through Session getters. this.jdbcCoordinator = new JdbcCoordinatorImpl( options.getConnection(), this, fastSessionServices.jdbcServices ); this.transactionCoordinator = fastSessionServices.transactionCoordinatorBuilder.buildTransactionCoordinator( jdbcCoordinator, this ); } @@ -1014,7 +1023,8 @@ public abstract class AbstractSharedSessionContract implements SharedSessionCont factory = SessionFactoryImpl.deserialize( ois ); fastSessionServices = factory.getFastSessionServices(); sessionEventsManager = new SessionEventListenerManagerImpl( fastSessionServices.defaultSessionEventListeners.buildBaseline() ); - jdbcSessionContext = new JdbcSessionContextImpl( this, (StatementInspector) ois.readObject(), fastSessionServices ); + jdbcSessionContext = new JdbcSessionContextImpl( this, (StatementInspector) ois.readObject(), + connectionHandlingMode, fastSessionServices ); jdbcCoordinator = JdbcCoordinatorImpl.deserialize( ois, this ); cacheTransactionSync = factory.getCache().getRegionFactory().createTransactionContext( this ); diff --git a/hibernate-core/src/main/java/org/hibernate/internal/JdbcSessionContextImpl.java b/hibernate-core/src/main/java/org/hibernate/internal/JdbcSessionContextImpl.java index 954593d6d5..36d16b05af 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/JdbcSessionContextImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/JdbcSessionContextImpl.java @@ -31,10 +31,11 @@ public class JdbcSessionContextImpl implements JdbcSessionContext { public JdbcSessionContextImpl( SharedSessionContractImplementor session, StatementInspector statementInspector, + PhysicalConnectionHandlingMode connectionHandlingMode, FastSessionServices fastSessionServices) { this.sessionFactory = session.getFactory(); this.statementInspector = statementInspector; - this.connectionHandlingMode = settings().getPhysicalConnectionHandlingMode(); + this.connectionHandlingMode = connectionHandlingMode; this.serviceRegistry = sessionFactory.getServiceRegistry(); this.jdbcObserver = new JdbcObserverImpl( session, fastSessionServices ); diff --git a/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java b/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java index ea8a319f1c..4a609a376a 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java @@ -1431,7 +1431,7 @@ public class SessionFactoryImpl implements SessionFactoryImplementor { @Override public PhysicalConnectionHandlingMode getPhysicalConnectionHandlingMode() { - return null; + return sessionFactory.getSessionFactoryOptions().getPhysicalConnectionHandlingMode(); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/resource/jdbc/internal/LogicalConnectionManagedImpl.java b/hibernate-core/src/main/java/org/hibernate/resource/jdbc/internal/LogicalConnectionManagedImpl.java index b6cd2c01cc..642cadcfa6 100644 --- a/hibernate-core/src/main/java/org/hibernate/resource/jdbc/internal/LogicalConnectionManagedImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/resource/jdbc/internal/LogicalConnectionManagedImpl.java @@ -193,33 +193,34 @@ public class LogicalConnectionManagedImpl extends AbstractLogicalConnectionImple } private void releaseConnection() { - //Some managed containers might trigger this release concurrently: - //this is not how they should do things, still we make a local - //copy of the variable to prevent confusing errors due to a race conditions - //(to trigger a more clear error, if any). final Connection localVariableConnection = this.physicalConnection; if ( localVariableConnection == null ) { return; } + // We need to set the connection to null before we release resources, + // in order to prevent recursion into this method. + // Recursion can happen when we release resources and when batch statements are in progress: + // when releasing resources, we'll abort the batch statement, + // which will trigger "logicalConnection.afterStatement()", + // which in some configurations will release the connection. + this.physicalConnection = null; try { - if ( ! localVariableConnection.isClosed() ) { - sqlExceptionHelper.logAndClearWarnings( localVariableConnection ); + try { + getResourceRegistry().releaseResources(); + if ( !localVariableConnection.isClosed() ) { + sqlExceptionHelper.logAndClearWarnings( localVariableConnection ); + } + } + finally { + jdbcConnectionAccess.releaseConnection( localVariableConnection ); } - jdbcConnectionAccess.releaseConnection( localVariableConnection ); } catch (SQLException e) { throw sqlExceptionHelper.convert( e, "Unable to release JDBC Connection" ); } finally { observer.jdbcConnectionReleaseEnd(); - boolean concurrentUsageDetected = ( this.physicalConnection == null ); - this.physicalConnection = null; - getResourceRegistry().releaseResources(); - if ( concurrentUsageDetected ) { - throw new HibernateException( "Detected concurrent management of connection resources." + - " This might indicate a multi-threaded use of Hibernate in combination with managed resources, which is not supported." ); - } } } diff --git a/hibernate-core/src/test/java/org/hibernate/test/connections/BeforeCompletionReleaseTest.java b/hibernate-core/src/test/java/org/hibernate/test/connections/BeforeCompletionReleaseTest.java index f932b558fe..4d50804aef 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/connections/BeforeCompletionReleaseTest.java +++ b/hibernate-core/src/test/java/org/hibernate/test/connections/BeforeCompletionReleaseTest.java @@ -7,46 +7,98 @@ package org.hibernate.test.connections; -import org.hibernate.cfg.AvailableSettings; -import org.hibernate.dialect.H2Dialect; -import org.hibernate.engine.jdbc.connections.internal.UserSuppliedConnectionProviderImpl; -import org.hibernate.engine.jdbc.connections.spi.ConnectionProvider; -import org.hibernate.jpa.test.BaseEntityManagerFunctionalTestCase; -import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; -import org.hibernate.testing.RequiresDialect; -import org.hibernate.testing.env.ConnectionProviderBuilder; -import org.hibernate.testing.jta.TestingJtaBootstrap; -import org.hibernate.testing.jta.TestingJtaPlatformImpl; -import org.hibernate.testing.transaction.TransactionUtil; -import org.junit.Test; - +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Arrays; +import java.util.List; +import java.util.Map; import javax.persistence.Entity; import javax.persistence.Id; import javax.persistence.Table; import javax.transaction.RollbackException; import javax.transaction.SystemException; -import javax.transaction.Transaction; +import javax.transaction.xa.XAException; import javax.transaction.xa.XAResource; -import javax.transaction.xa.Xid; -import java.sql.Connection; -import java.sql.SQLException; -import java.util.Map; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import org.hibernate.Session; +import org.hibernate.cfg.AvailableSettings; +import org.hibernate.dialect.H2Dialect; +import org.hibernate.engine.jdbc.connections.internal.UserSuppliedConnectionProviderImpl; +import org.hibernate.engine.jdbc.connections.spi.ConnectionProvider; +import org.hibernate.engine.spi.SessionImplementor; +import org.hibernate.jpa.test.BaseEntityManagerFunctionalTestCase; +import org.hibernate.resource.jdbc.spi.LogicalConnectionImplementor; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; + +import org.hibernate.testing.RequiresDialect; +import org.hibernate.testing.TestForIssue; +import org.hibernate.testing.env.ConnectionProviderBuilder; +import org.hibernate.testing.jta.TestingJtaBootstrap; +import org.hibernate.testing.jta.TestingJtaPlatformImpl; +import org.hibernate.testing.junit4.CustomParameterized; +import org.hibernate.testing.transaction.TransactionUtil2; +import org.junit.Rule; +import org.junit.Test; + +import org.mockito.InOrder; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.mockito.quality.Strictness; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; /** * @author Luis Barreiro */ @RequiresDialect( H2Dialect.class ) +@RunWith(CustomParameterized.class) public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTestCase { + @Parameterized.Parameters(name = "{0}") + public static List params() { + return Arrays.asList( new Object[][] { + { + "Setting connection handling mode from properties", + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION, + null + }, + { + "Setting connection handling mode through SessionBuilder", + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_AFTER_STATEMENT, + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION + } + } ); + } + + @Rule + public MockitoRule mockito = MockitoJUnit.rule().strictness( Strictness.STRICT_STUBS ); + + private final PhysicalConnectionHandlingMode connectionHandlingModeInProperties; + private final PhysicalConnectionHandlingMode connectionHandlingModeInSessionBuilder; + + public BeforeCompletionReleaseTest( + String ignoredTestLabel, PhysicalConnectionHandlingMode connectionHandlingModeInProperties, + PhysicalConnectionHandlingMode connectionHandlingModeInSessionBuilder) { + this.connectionHandlingModeInProperties = connectionHandlingModeInProperties; + this.connectionHandlingModeInSessionBuilder = connectionHandlingModeInSessionBuilder; + } + @Override protected Map getConfig() { Map config = super.getConfig(); TestingJtaBootstrap.prepare( config ); config.put( AvailableSettings.CONNECTION_PROVIDER, new ConnectionProviderDecorator() ); - config.put( AvailableSettings.CONNECTION_HANDLING, PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION ); + if ( connectionHandlingModeInProperties != null ) { + config.put( AvailableSettings.CONNECTION_HANDLING, connectionHandlingModeInProperties ); + } return config; } @@ -56,12 +108,49 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest } @Test - public void testConnectionAcquisitionCount() { - TransactionUtil.doInJPA( this::entityManagerFactory, entityManager -> { - Thing thing = new Thing(); - thing.setId( 1 ); - entityManager.persist( thing ); - }); + @TestForIssue(jiraKey = {"HHH-13976", "HHH-14326"}) + public void testResourcesReleasedThenConnectionClosedThenCommit() throws SQLException, XAException { + XAResource transactionSpy = mock( XAResource.class ); + Connection[] connectionSpies = new Connection[1]; + Statement statementMock = Mockito.mock( Statement.class ); + + try (SessionImplementor s = (SessionImplementor) openSession()) { + TransactionUtil2.inTransaction( s, session -> { + spyOnTransaction( transactionSpy ); + + Thing thing = new Thing(); + thing.setId( 1 ); + session.persist( thing ); + + LogicalConnectionImplementor logicalConnection = session.getJdbcCoordinator().getLogicalConnection(); + logicalConnection.getResourceRegistry().register( statementMock, true ); + connectionSpies[0] = logicalConnection.getPhysicalConnection(); + } ); + } + + Connection connectionSpy = connectionSpies[0]; + + // Must close the resources, then the connection, then commit + InOrder inOrder = inOrder( statementMock, connectionSpy, transactionSpy ); + inOrder.verify( statementMock ).close(); + inOrder.verify( connectionSpy ).close(); + inOrder.verify( transactionSpy ).commit( any(), anyBoolean() ); + } + + private void spyOnTransaction(XAResource xaResource) { + try { + TestingJtaPlatformImpl.transactionManager().getTransaction().enlistResource( xaResource ); + } + catch (RollbackException | SystemException e) { + throw new IllegalStateException( e ); + } + } + + private Session openSession() { + return connectionHandlingModeInSessionBuilder == null + ? entityManagerFactory().openSession() + : entityManagerFactory().withOptions().connectionHandlingMode( connectionHandlingModeInSessionBuilder ) + .openSession(); } // --- // @@ -94,63 +183,7 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest @Override public Connection getConnection() throws SQLException { - Connection connection = dataSource.getConnection(); - - try { - Transaction tx = TestingJtaPlatformImpl.transactionManager().getTransaction(); - if ( tx != null) { - tx.enlistResource( new XAResource() { - - @Override public void commit(Xid xid, boolean onePhase) { - try { - assertTrue( "Connection should be closed prior to commit", connection.isClosed() ); - } catch ( SQLException e ) { - fail( "Unexpected SQLException: " + e.getMessage() ); - } - } - - @Override public void end(Xid xid, int flags) { - } - - @Override public void forget(Xid xid) { - } - - @Override public int getTransactionTimeout() { - return 0; - } - - @Override public boolean isSameRM(XAResource xares) { - return false; - } - - @Override public int prepare(Xid xid) { - return 0; - } - - @Override public Xid[] recover(int flag) { - return new Xid[0]; - } - - @Override public void rollback(Xid xid) { - try { - assertTrue( "Connection should be closed prior to rollback", connection.isClosed() ); - } catch ( SQLException e ) { - fail( "Unexpected SQLException: " + e.getMessage() ); - } - } - - @Override public boolean setTransactionTimeout(int seconds) { - return false; - } - - @Override public void start(Xid xid, int flags) { - } - }); - } - } catch ( SystemException | RollbackException e ) { - fail( e.getMessage() ); - } - return connection; + return spy( dataSource.getConnection() ); } @Override @@ -159,3 +192,4 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest } } } + diff --git a/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ChildEntity.java b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ChildEntity.java new file mode 100644 index 0000000000..89c549ddcf --- /dev/null +++ b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ChildEntity.java @@ -0,0 +1,23 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.id.hhh14407; + +import javax.persistence.Basic; +import javax.persistence.Column; +import javax.persistence.Entity; + +/** + * @author Sönke Reimer + */ +@Entity(name="ChildEntity") +class ChildEntity extends ParentEntity { + + @Basic + @Column(name="CHILD") + private String ivChild; + +} diff --git a/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ParentEntity.java b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ParentEntity.java new file mode 100644 index 0000000000..b95da7ca48 --- /dev/null +++ b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/ParentEntity.java @@ -0,0 +1,34 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.id.hhh14407; + +import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.Inheritance; +import javax.persistence.InheritanceType; +import javax.persistence.Version; + +/** + * @author Sönke Reimer + */ +@Entity(name="ParentEntity") +@Inheritance(strategy=InheritanceType.TABLE_PER_CLASS) +class ParentEntity { + + @Id + @Column(name = "ID", length = 32) + private String Id; + + @Version + @Column(name = "LOCK_VERSION") + private int Lock_Version; + public String getId() { + return Id; + } + +} diff --git a/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java new file mode 100644 index 0000000000..85a2a64095 --- /dev/null +++ b/hibernate-core/src/test_legacy/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java @@ -0,0 +1,58 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.id.hhh14407; + +import org.hibernate.cfg.Configuration; +import org.hibernate.cfg.Environment; +import org.hibernate.dialect.H2Dialect; +import org.hibernate.hql.spi.id.MultiTableBulkIdStrategy; +import org.hibernate.hql.spi.id.persistent.PersistentTableBulkIdStrategy; + +import org.hibernate.testing.RequiresDialect; +import org.hibernate.testing.TestForIssue; +import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase; +import org.junit.Test; + +/** + * @author Nathan Xu + * @author Sönke Reimer + */ +@RequiresDialect( value = H2Dialect.class ) +@TestForIssue( jiraKey = "HHH14407" ) +public class PersistentTableBulkIdStrategyNPETest extends BaseCoreFunctionalTestCase { + + @Override + protected void configure(Configuration configuration) { + configuration.setProperty( + Environment.DIALECT, + PersistentTableBulkIdH2Dialect.class.getName() + ); + } + + @Override + protected Class[] getAnnotatedClasses() { + return new Class[] { + ParentEntity.class, + ChildEntity.class + }; + } + + @Test + public void hhh14407Test() { + // without fix of HHH14407, the test case will trigger exception due to NPE in PersistentTableBulkIdStrategy + } + + public static class PersistentTableBulkIdH2Dialect extends H2Dialect { + + @Override + public MultiTableBulkIdStrategy getDefaultMultiTableBulkIdStrategy() { + return new PersistentTableBulkIdStrategy(); + } + + } + +} diff --git a/hibernate-testing/src/main/java/org/hibernate/testing/TestForIssue.java b/hibernate-testing/src/main/java/org/hibernate/testing/TestForIssue.java index ebe0f4d669..7d76c0e59e 100644 --- a/hibernate-testing/src/main/java/org/hibernate/testing/TestForIssue.java +++ b/hibernate-testing/src/main/java/org/hibernate/testing/TestForIssue.java @@ -23,5 +23,5 @@ public @interface TestForIssue { * The key of a JIRA issue tested. * @return The jira issue key */ - String jiraKey(); + String[] jiraKey(); }