From ee9b3585c525273a1ff7a85c0ca5ee093f5a1e3f Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Thu, 15 Mar 2012 10:14:08 -0500 Subject: [PATCH] HHH-7020 - Connection leak with nested sessions --- .../internal/JdbcResourceRegistryImpl.java | 73 ++--------- .../java/org/hibernate/IrrelevantEntity.java | 60 +++++++++ .../SessionWithSharedConnectionTest.java | 123 ++++++++++++++++++ 3 files changed, 197 insertions(+), 59 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/IrrelevantEntity.java create mode 100644 hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/internal/JdbcResourceRegistryImpl.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/internal/JdbcResourceRegistryImpl.java index 34a28693d6..74688663ee 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/internal/JdbcResourceRegistryImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/internal/JdbcResourceRegistryImpl.java @@ -26,6 +26,7 @@ package org.hibernate.engine.jdbc.internal; import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; +import java.util.ConcurrentModificationException; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -174,72 +175,20 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { private void cleanup() { for ( Map.Entry> entry : xref.entrySet() ) { if ( entry.getValue() != null ) { - for ( ResultSet resultSet : entry.getValue() ) { - close( resultSet ); - } - entry.getValue().clear(); + closeAll( entry.getValue() ); } close( entry.getKey() ); } xref.clear(); - for ( ResultSet resultSet : unassociatedResultSets ) { + closeAll( unassociatedResultSets ); + } + + protected void closeAll(Set resultSets) { + for ( ResultSet resultSet : resultSets ) { close( resultSet ); } - unassociatedResultSets.clear(); - - // TODO: can ConcurrentModificationException still happen??? - // Following is from old AbstractBatcher... - /* - Iterator iter = resultSetsToClose.iterator(); - while ( iter.hasNext() ) { - try { - logCloseResults(); - ( ( ResultSet ) iter.next() ).close(); - } - catch ( SQLException e ) { - // no big deal - log.warn( "Could not close a JDBC result set", e ); - } - catch ( ConcurrentModificationException e ) { - // this has been shown to happen occasionally in rare cases - // when using a transaction manager + transaction-timeout - // where the timeout calls back through Hibernate's - // registered transaction synchronization on a separate - // "reaping" thread. In cases where that reaping thread - // executes through this block at the same time the main - // application thread does we can get into situations where - // these CMEs occur. And though it is not "allowed" per-se, - // the end result without handling it specifically is infinite - // looping. So here, we simply break the loop - log.info( "encountered CME attempting to release batcher; assuming cause is tx-timeout scenario and ignoring" ); - break; - } - catch ( Throwable e ) { - // sybase driver (jConnect) throwing NPE here in certain - // cases, but we'll just handle the general "unexpected" case - log.warn( "Could not close a JDBC result set", e ); - } - } - resultSetsToClose.clear(); - - iter = statementsToClose.iterator(); - while ( iter.hasNext() ) { - try { - closeQueryStatement( ( PreparedStatement ) iter.next() ); - } - catch ( ConcurrentModificationException e ) { - // see explanation above... - log.info( "encountered CME attempting to release batcher; assuming cause is tx-timeout scenario and ignoring" ); - break; - } - catch ( SQLException e ) { - // no big deal - log.warn( "Could not close a JDBC statement", e ); - } - } - statementsToClose.clear(); - */ + resultSets.clear(); } public void close() { @@ -296,6 +245,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { InvalidatableWrapper wrapper = (InvalidatableWrapper) resultSet; close( wrapper.getWrappedObject() ); wrapper.invalidate(); + return; } try { @@ -306,5 +256,10 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { LOG.debugf( "Unable to release result set [%s]", e.getMessage() ); } } + catch ( Exception e ) { + // sybase driver (jConnect) throwing NPE here in certain cases, but we'll just handle the + // general "unexpected" case + LOG.debugf( "Could not close a JDBC result set [%s]", e.getMessage() ); + } } } diff --git a/hibernate-core/src/test/java/org/hibernate/IrrelevantEntity.java b/hibernate-core/src/test/java/org/hibernate/IrrelevantEntity.java new file mode 100644 index 0000000000..94a0a878ae --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/IrrelevantEntity.java @@ -0,0 +1,60 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * Copyright (c) 2012, Red Hat Inc. or third-party contributors as + * indicated by the @author tags or express copyright attribution + * statements applied by the authors. All third-party contributions are + * distributed under license by Red Hat Inc. + * + * This copyrighted material is made available to anyone wishing to use, modify, + * copy, or redistribute it subject to the terms and conditions of the GNU + * Lesser General Public License, as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this distribution; if not, write to: + * Free Software Foundation, Inc. + * 51 Franklin Street, Fifth Floor + * Boston, MA 02110-1301 USA + */ +package org.hibernate; + +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.Id; + +import org.hibernate.annotations.GenericGenerator; + +/** + * A testing entity for cases where the entity definition itself is irrelevant (testing JDBC connection semantics, etc). + * + * @author Steve Ebersole + */ +@Entity +public class IrrelevantEntity { + private Integer id; + private String name; + + @Id + @GeneratedValue( generator = "increment" ) + @GenericGenerator( name = "increment", strategy = "increment" ) + public Integer getId() { + return id; + } + + public void setId(Integer id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java b/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java new file mode 100644 index 0000000000..200d425279 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java @@ -0,0 +1,123 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * Copyright (c) 2012, Red Hat Inc. or third-party contributors as + * indicated by the @author tags or express copyright attribution + * statements applied by the authors. All third-party contributions are + * distributed under license by Red Hat Inc. + * + * This copyrighted material is made available to anyone wishing to use, modify, + * copy, or redistribute it subject to the terms and conditions of the GNU + * Lesser General Public License, as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this distribution; if not, write to: + * Free Software Foundation, Inc. + * 51 Franklin Street, Fifth Floor + * Boston, MA 02110-1301 USA + */ +package org.hibernate.sharedSession; + +import org.hibernate.Criteria; +import org.hibernate.IrrelevantEntity; +import org.hibernate.Session; +import org.hibernate.engine.spi.SessionImplementor; + +import org.junit.Test; + +import org.hibernate.testing.FailureExpected; +import org.hibernate.testing.TestForIssue; +import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * @author Steve Ebersole + */ +public class SessionWithSharedConnectionTest extends BaseCoreFunctionalTestCase { + @Test + @TestForIssue( jiraKey = "HHH-7020" ) + @FailureExpected( jiraKey = "HHH-7020" ) + public void testSharedTransactionContextSessionClosing() { + Session session = sessionFactory().openSession(); + session.getTransaction().begin(); + + Session secondSession = session.sessionWithOptions() + .connection() + .openSession(); + secondSession.createCriteria( IrrelevantEntity.class ).list(); + + //the list should have registered and then released a JDBC resource + assertFalse( + ((SessionImplementor) secondSession).getTransactionCoordinator() + .getJdbcCoordinator() + .getLogicalConnection() + .getResourceRegistry() + .hasRegisteredResources() + ); + + //there should be no registered JDBC resources on the original session +// not sure this is ultimately a valid assertion +// assertFalse( +// ((SessionImplementor) session).getTransactionCoordinator() +// .getJdbcCoordinator() +// .getLogicalConnection() +// .getResourceRegistry() +// .hasRegisteredResources() +// ); + + secondSession.close(); + + session.getTransaction().commit(); + //the session should be allowed to close properly as well + session.close(); + } + + @Test + @TestForIssue( jiraKey = "HHH-7020" ) + @FailureExpected( jiraKey = "HHH-7020" ) + public void testSharedTransactionContextAutoClosing() { + Session session = sessionFactory().openSession(); + session.getTransaction().begin(); + + Session secondSession = session.sessionWithOptions() + .connection() + .autoClose( true ) + .openSession(); + secondSession.createCriteria( IrrelevantEntity.class ).list(); + + //the list should have registered and then released a JDBC resource + assertFalse( + ((SessionImplementor) secondSession).getTransactionCoordinator() + .getJdbcCoordinator() + .getLogicalConnection() + .getResourceRegistry() + .hasRegisteredResources() + ); + //there should be no registered JDBC resources on the original session +// not sure this is ultimately a valid assertion +// assertFalse( +// ((SessionImplementor) session).getTransactionCoordinator() +// .getJdbcCoordinator() +// .getLogicalConnection() +// .getResourceRegistry() +// .hasRegisteredResources() +// ); + + session.getTransaction().commit(); + + assertTrue( ((SessionImplementor) session).isClosed() ); + assertTrue( ((SessionImplementor) secondSession).isClosed() ); + } + + @Override + protected Class[] getAnnotatedClasses() { + return new Class[] { IrrelevantEntity.class }; + } +}