HHH-14326 + HHH-14404 Connection closing fixes

hibernate-core/src/main/java/org/hibernate/resource/jdbc/internal/LogicalConnectionManagedImpl.java

@@ -194,33 +194,42 @@ public void manualReconnect(Connection suppliedConnection) {
 	}
 
 	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 {
-			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." );
-			}
 		}
 	}
 

hibernate-core/src/test/java/org/hibernate/test/connections/BeforeCompletionReleaseTest.java

@@ -7,40 +7,55 @@
 
 package org.hibernate.test.connections;
 
+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.xa.XAException;
+import javax.transaction.xa.XAResource;
+
 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.TransactionUtil;
+import org.hibernate.testing.transaction.TransactionUtil2;
+import org.junit.Rule;
 import org.junit.Test;
 
-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.XAResource;
-import javax.transaction.xa.Xid;
-import java.sql.Connection;
-import java.sql.SQLException;
-import java.util.Map;
+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.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+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 )
 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 @@ protected Class<?>[] getAnnotatedClasses() {
     }
 
     @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 ConnectionProviderDecorator() {
 
         @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

hibernate-testing/src/main/java/org/hibernate/testing/TestForIssue.java

@@ -23,5 +23,5 @@
 	 * The key of a JIRA issue tested.
 	 * @return The jira issue key
 	 */
-	String jiraKey();
+	String[] jiraKey();
 }