From 5d6d9b87c11c56bdfa51b3d35eaafa57d3f784fb Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Mon, 19 Mar 2012 19:09:44 -0500 Subject: [PATCH] HHH-7020 - Connection leak with nested sessions --- .../internal/JdbcResourceRegistryImpl.java | 33 +++++--- .../engine/jdbc/spi/JdbcResourceRegistry.java | 27 +++++-- .../org/hibernate/internal/SessionImpl.java | 15 ++-- .../SessionWithSharedConnectionTest.java | 75 ------------------- 4 files changed, 50 insertions(+), 100 deletions(-) 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 74688663ee..aa9937d364 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,7 +26,6 @@ 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; @@ -61,6 +60,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { this.exceptionHelper = exceptionHelper; } + @Override public void register(Statement statement) { LOG.tracev( "Registering statement [{0}]", statement ); if ( xref.containsKey( statement ) ) { @@ -69,6 +69,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { xref.put( statement, null ); } + @Override @SuppressWarnings({ "unchecked" }) public void registerLastQuery(Statement statement) { LOG.tracev( "Registering last query statement [{0}]", statement ); @@ -80,6 +81,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { lastQuery = statement; } + @Override public void cancelLastQuery() { try { if (lastQuery != null) { @@ -97,6 +99,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { } } + @Override public void release(Statement statement) { LOG.tracev( "Releasing statement [{0}]", statement ); Set resultSets = xref.get( statement ); @@ -110,6 +113,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { close( statement ); } + @Override public void register(ResultSet resultSet) { LOG.tracev( "Registering result set [{0}]", resultSet ); Statement statement; @@ -135,6 +139,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { } } + @Override public void release(ResultSet resultSet) { LOG.tracev( "Releasing result set [{0}]", resultSet ); Statement statement; @@ -158,15 +163,19 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { } else { boolean removed = unassociatedResultSets.remove( resultSet ); - if (!removed) LOG.unregisteredResultSetWithoutStatement(); + if ( !removed ) { + LOG.unregisteredResultSetWithoutStatement(); + } } close( resultSet ); } + @Override public boolean hasRegisteredResources() { return ! xref.isEmpty() || ! unassociatedResultSets.isEmpty(); } + @Override public void releaseResources() { LOG.tracev( "Releasing JDBC container resources [{0}]", this ); cleanup(); @@ -191,6 +200,7 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { resultSets.clear(); } + @Override public void close() { LOG.tracev( "Closing JDBC container [{0}]", this ); cleanup(); @@ -230,10 +240,12 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { lastQuery = null; } } - catch( SQLException sqle ) { - if ( LOG.isDebugEnabled() ) { - LOG.debugf( "Unable to release statement [%s]", sqle.getMessage() ); - } + catch( SQLException e ) { + LOG.debugf( "Unable to release JDBC statement [%s]", e.getMessage() ); + } + catch ( Exception e ) { + // try to handle general errors more elegantly + LOG.debugf( "Unable to release JDBC statement [%s]", e.getMessage() ); } } @@ -252,14 +264,11 @@ public class JdbcResourceRegistryImpl implements JdbcResourceRegistry { resultSet.close(); } catch( SQLException e ) { - if ( LOG.isDebugEnabled() ) { - LOG.debugf( "Unable to release result set [%s]", e.getMessage() ); - } + LOG.debugf( "Unable to release JDBC 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() ); + // try to handle general errors more elegantly + LOG.debugf( "Unable to release JDBC result set [%s]", e.getMessage() ); } } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/spi/JdbcResourceRegistry.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/spi/JdbcResourceRegistry.java index 4ec5c57ee3..7596322428 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/spi/JdbcResourceRegistry.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/spi/JdbcResourceRegistry.java @@ -28,7 +28,16 @@ import java.sql.ResultSet; import java.sql.Statement; /** - * Defines a registry of JDBC resources related to a particular unit of work. + * Defines a registry of JDBC resources related to a particular unit of work. The main function of a + * JdbcResourceRegistry is to make sure resources get cleaned up. This is accomplished by registering all + * JDBC-related resources via the {@link #register(java.sql.Statement)} and {@link #register(java.sql.ResultSet)} + * methods. When done with these resources, they should be released by the corollary + * {@link #release(java.sql.Statement)} and {@link #release(java.sql.ResultSet)} methods. Any un-released resources + * will be released automatically when this registry is closed via {@link #close()}. Additionally, + * all registered resources can be released at any time using {@link #releaseResources()}. + *

+ * Additionally, a query can be registered as being able to be cancelled via the {@link #registerLastQuery} + * method. Such statements can then be cancelled by calling {@link #cancelLastQuery()} * * @author Steve Ebersole */ @@ -39,10 +48,6 @@ public interface JdbcResourceRegistry extends Serializable { * @param statement The statement to register. */ public void register(Statement statement); - - public void registerLastQuery(Statement statement); - - public void cancelLastQuery(); /** * Release a previously registered statement. @@ -83,4 +88,16 @@ public interface JdbcResourceRegistry extends Serializable { * After execution, the registry is considered unusable. */ public void close(); + + /** + * Register a query statement as being able to be cancelled. + * + * @param statement The cancel-able query statement. + */ + public void registerLastQuery(Statement statement); + + /** + * Cancel the last query registered via {@link #registerLastQuery} + */ + public void cancelLastQuery(); } diff --git a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java index a3fcaffad4..3de33a68cc 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java @@ -2193,12 +2193,8 @@ public final class SessionImpl extends AbstractSessionImpl implements EventSourc @Override public SharedSessionBuilder connection() { - return connection( - session.transactionCoordinator - .getJdbcCoordinator() - .getLogicalConnection() - .getDistinctConnectionProxy() - ); + this.shareTransactionContext = true; + return this; } @Override @@ -2221,10 +2217,13 @@ public final class SessionImpl extends AbstractSessionImpl implements EventSourc return flushBeforeCompletion( session.flushBeforeCompletionEnabled ); } + /** + * @deprecated Use {@link #connection()} instead + */ @Override + @Deprecated public SharedSessionBuilder transactionContext() { - this.shareTransactionContext = true; - return this; + return connection(); } @Override diff --git a/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java b/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java index ecf135d0d1..4a078e79b9 100644 --- a/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java +++ b/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java @@ -44,81 +44,6 @@ 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 testSharedConnectionSessionClosing() { - 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 testSharedConnectionAutoClosing() { - 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() ); - } - @Test @TestForIssue( jiraKey = "HHH-7090" ) public void testSharedTransactionContextSessionClosing() {