From 1e5d64cf7944c6a4262c5a9408c6c9442ff585a2 Mon Sep 17 00:00:00 2001 From: Nathan Xu Date: Thu, 14 Jan 2021 22:43:56 -0500 Subject: [PATCH 1/7] HHH-14407 NPE in Column.getSqlTypeCode(Mapping mapping) for column 'hib_sess_id' when using PersistentTableBulkIdStrategy --- .../AbstractMultiTableBulkIdStrategyImpl.java | 6 +- .../PersistentTableBulkIdStrategy.java | 3 + .../hibernate/id/hhh14407/ChildEntity.java | 23 ++++++++ .../hibernate/id/hhh14407/ParentEntity.java | 34 +++++++++++ .../PersistentTableBulkIdStrategyNPETest.java | 58 +++++++++++++++++++ 5 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/id/hhh14407/ChildEntity.java create mode 100644 hibernate-core/src/test/java/org/hibernate/id/hhh14407/ParentEntity.java create mode 100644 hibernate-core/src/test/java/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractMultiTableBulkIdStrategyImpl.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractMultiTableBulkIdStrategyImpl.java index 3b7fa3c7b4..930da89d34 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractMultiTableBulkIdStrategyImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractMultiTableBulkIdStrategyImpl.java @@ -137,13 +137,13 @@ public abstract class AbstractMultiTableBulkIdStrategyImpl itr = idTable.getColumnIterator(); while ( itr.hasNext() ) { - final Column column = (Column) itr.next(); + final Column column = itr.next(); buffer.append( column.getQuotedName( dialect ) ).append( ' ' ); buffer.append( column.getSqlType( dialect, metadata ) ); - final int sqlTypeCode = column.getSqlTypeCode( metadata ); + final int sqlTypeCode = column.getSqlTypeCode() != null ? column.getSqlTypeCode() : column.getSqlTypeCode( metadata ); final String columnAnnotation = dialect.getCreateTemporaryTableColumnAnnotation( sqlTypeCode ); if ( !columnAnnotation.isEmpty() ) { buffer.append(" ").append( columnAnnotation ); diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/PersistentTableBulkIdStrategy.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/PersistentTableBulkIdStrategy.java index bfc7b2da18..04c2cf0c04 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/PersistentTableBulkIdStrategy.java +++ b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/PersistentTableBulkIdStrategy.java @@ -6,6 +6,8 @@ */ package org.hibernate.hql.spi.id.persistent; +import java.sql.Types; + import org.hibernate.boot.model.naming.Identifier; import org.hibernate.boot.model.relational.QualifiedTableName; import org.hibernate.boot.registry.StandardServiceRegistry; @@ -111,6 +113,7 @@ public class PersistentTableBulkIdStrategy protected void augmentIdTableDefinition(Table idTable) { Column sessionIdColumn = new Column( Helper.SESSION_ID_COLUMN_NAME ); sessionIdColumn.setSqlType( "CHAR(36)" ); + sessionIdColumn.setSqlTypeCode( Types.VARCHAR ); sessionIdColumn.setComment( "Used to hold the Hibernate Session identifier" ); idTable.addColumn( sessionIdColumn ); } diff --git a/hibernate-core/src/test/java/org/hibernate/id/hhh14407/ChildEntity.java b/hibernate-core/src/test/java/org/hibernate/id/hhh14407/ChildEntity.java new file mode 100644 index 0000000000..89c549ddcf --- /dev/null +++ b/hibernate-core/src/test/java/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/java/org/hibernate/id/hhh14407/ParentEntity.java b/hibernate-core/src/test/java/org/hibernate/id/hhh14407/ParentEntity.java new file mode 100644 index 0000000000..b95da7ca48 --- /dev/null +++ b/hibernate-core/src/test/java/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/java/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java b/hibernate-core/src/test/java/org/hibernate/id/hhh14407/PersistentTableBulkIdStrategyNPETest.java new file mode 100644 index 0000000000..85a2a64095 --- /dev/null +++ b/hibernate-core/src/test/java/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(); + } + + } + +} From 74433cdec33655fad911563a8439cee40853c2b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 10:34:51 +0100 Subject: [PATCH 2/7] HHH-14326 Release JDBC resources before closing the connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../LogicalConnectionManagedImpl.java | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) 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 b51ad13a91..3584b6d6c3 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 @@ -194,16 +194,28 @@ 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. + + //Some managed containers might trigger this release concurrently: + //this is not how they should do things, still we try to detect it to trigger a more clear error. + boolean concurrentUsageDetected = ( this.physicalConnection == null ); + this.physicalConnection = null; + 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." ); + } try { + getResourceRegistry().releaseResources(); if ( ! localVariableConnection.isClosed() ) { sqlExceptionHelper.logAndClearWarnings( localVariableConnection ); } @@ -214,13 +226,6 @@ public class LogicalConnectionManagedImpl extends AbstractLogicalConnectionImple } 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." ); - } } } From e5c830da19cbfa57c5a23b2d884244f280d586e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 10:36:15 +0100 Subject: [PATCH 3/7] HHH-14326 Always close the connection even if releasing JDBC resources fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../jdbc/internal/LogicalConnectionManagedImpl.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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 3584b6d6c3..d7f45d0636 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 @@ -215,11 +215,15 @@ public class LogicalConnectionManagedImpl extends AbstractLogicalConnectionImple " This might indicate a multi-threaded use of Hibernate in combination with managed resources, which is not supported." ); } try { - getResourceRegistry().releaseResources(); - 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" ); From d726dcb394e69eec02af2c8c3ac3ebc6679dc19c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 13:36:51 +0100 Subject: [PATCH 4/7] HHH-14326 Test JDBC resources are released before closing the connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../BeforeCompletionReleaseTest.java | 149 ++++++++---------- .../org/hibernate/testing/TestForIssue.java | 2 +- 2 files changed, 69 insertions(+), 82 deletions(-) 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..91ed0e6d66 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,33 +7,45 @@ 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.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.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.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.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 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 @@ -41,6 +53,9 @@ import static org.junit.Assert.fail; @RequiresDialect( H2Dialect.class ) public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTestCase { + @Rule + public MockitoRule mockito = MockitoJUnit.rule().strictness( Strictness.STRICT_STUBS ); + @Override protected Map getConfig() { Map config = super.getConfig(); @@ -56,12 +71,40 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest } @Test - public void testConnectionAcquisitionCount() { - TransactionUtil.doInJPA( this::entityManagerFactory, entityManager -> { + @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 ); + + TransactionUtil2.inTransaction( entityManagerFactory(), session -> { + spyOnTransaction( transactionSpy ); + Thing thing = new Thing(); thing.setId( 1 ); - entityManager.persist( thing ); - }); + 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 ); + } } // --- // @@ -94,63 +137,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 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(); } From d0b44c48ef6926f59a265222040fafaedfca42a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 11:12:42 +0100 Subject: [PATCH 5/7] HHH-14404 Take into account the connectionHandlingMode passed through SessionBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../AbstractSharedSessionContract.java | 20 ++++++++++++++----- .../internal/JdbcSessionContextImpl.java | 3 ++- .../internal/SessionFactoryImpl.java | 2 +- 3 files changed, 18 insertions(+), 7 deletions(-) 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 9ad4e9f156..2fd16f8f8f 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/AbstractSharedSessionContract.java @@ -86,6 +86,7 @@ import org.hibernate.query.spi.NativeQueryImplementor; import org.hibernate.query.spi.QueryImplementor; import org.hibernate.query.spi.ScrollableResultsImplementor; 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; @@ -134,6 +135,7 @@ public abstract class AbstractSharedSessionContract implements SharedSessionCont private FlushMode flushMode; private boolean autoJoinTransactions; + private final PhysicalConnectionHandlingMode connectionHandlingMode; private CacheMode cacheMode; @@ -181,11 +183,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" ); @@ -207,18 +207,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 ); } @@ -1236,7 +1245,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 bbaf51baae..4f22a6a143 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/SessionFactoryImpl.java @@ -1473,7 +1473,7 @@ public class SessionFactoryImpl implements SessionFactoryImplementor { @Override public PhysicalConnectionHandlingMode getPhysicalConnectionHandlingMode() { - return null; + return sessionFactory.getSessionFactoryOptions().getPhysicalConnectionHandlingMode(); } @Override From 8210bc220b42774f311bd8fe7fb5d28936ab9fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 11:25:46 +0100 Subject: [PATCH 6/7] HHH-14404 Test setting the connection handling mode through SessionBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../BeforeCompletionReleaseTest.java | 67 ++++++++++++++++--- 1 file changed, 57 insertions(+), 10 deletions(-) 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 91ed0e6d66..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 @@ -10,6 +10,8 @@ package org.hibernate.test.connections; 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; @@ -19,18 +21,22 @@ import javax.transaction.SystemException; import javax.transaction.xa.XAException; import javax.transaction.xa.XAResource; +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; @@ -40,6 +46,8 @@ 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; @@ -51,17 +59,46 @@ 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; } @@ -77,17 +114,19 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest Connection[] connectionSpies = new Connection[1]; Statement statementMock = Mockito.mock( Statement.class ); - TransactionUtil2.inTransaction( entityManagerFactory(), session -> { - spyOnTransaction( transactionSpy ); + try (SessionImplementor s = (SessionImplementor) openSession()) { + TransactionUtil2.inTransaction( s, session -> { + spyOnTransaction( transactionSpy ); - Thing thing = new Thing(); - thing.setId( 1 ); - session.persist( thing ); + Thing thing = new Thing(); + thing.setId( 1 ); + session.persist( thing ); - LogicalConnectionImplementor logicalConnection = session.getJdbcCoordinator().getLogicalConnection(); - logicalConnection.getResourceRegistry().register( statementMock, true ); - connectionSpies[0] = logicalConnection.getPhysicalConnection(); - } ); + LogicalConnectionImplementor logicalConnection = session.getJdbcCoordinator().getLogicalConnection(); + logicalConnection.getResourceRegistry().register( statementMock, true ); + connectionSpies[0] = logicalConnection.getPhysicalConnection(); + } ); + } Connection connectionSpy = connectionSpies[0]; @@ -107,6 +146,13 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest } } + private Session openSession() { + return connectionHandlingModeInSessionBuilder == null + ? entityManagerFactory().openSession() + : entityManagerFactory().withOptions().connectionHandlingMode( connectionHandlingModeInSessionBuilder ) + .openSession(); + } + // --- // @Entity(name = "Thing") @@ -146,3 +192,4 @@ public class BeforeCompletionReleaseTest extends BaseEntityManagerFunctionalTest } } } + From 4c9c2a809ac6b3a3b54cd4a3cbedc53695ab9287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Wed, 20 Jan 2021 08:44:28 +0100 Subject: [PATCH 7/7] HHH-14404 Remove check for concurrent execution of LogicalConnectionManagedImpl#releaseConnection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's unlikely to ever happen, and even if it happened, the resulting exception would probably be rather clear about the fact that the problem is related to concurrent execution (ConcurrentModificationException thrown by a Map of resources, for example). See https://github.com/hibernate/hibernate-orm/pull/3693#discussion_r560393293 Signed-off-by: Yoann Rodière --- .../jdbc/internal/LogicalConnectionManagedImpl.java | 8 -------- 1 file changed, 8 deletions(-) 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 d7f45d0636..1d13e5d284 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 @@ -205,15 +205,7 @@ public class LogicalConnectionManagedImpl extends AbstractLogicalConnectionImple // when releasing resources, we'll abort the batch statement, // which will trigger "logicalConnection.afterStatement()", // which in some configurations will release the connection. - - //Some managed containers might trigger this release concurrently: - //this is not how they should do things, still we try to detect it to trigger a more clear error. - boolean concurrentUsageDetected = ( this.physicalConnection == null ); this.physicalConnection = null; - 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." ); - } try { try { getResourceRegistry().releaseResources();