Handle: unconditionally close Connection

Right now, we check if a connection is closed before closing it. Also, if a plugin
fails to customize a Handle, the connection can leak

However, this isn't good enough: even if the server closed the connection,
we might have borrowed this from a pool, which needs to know we've returned it.

Connection.close is specified to no-op if called more than once, so it's safe to just do it regardless.

Fixes #2446

RELEASE_NOTES.md

@@ -1,3 +1,6 @@
+# Unreleased
+  - Handle: fix potential Connection leak if connection is closed early
+
 # 3.40.0
 
   - add extension point to decorate withHandle and friends (#2448)

core/src/main/java/org/jdbi/v3/core/ConnectionFactory.java

@@ -45,6 +45,7 @@ default void closeConnection(Connection conn) throws SQLException {
 
     /**
      * Returns a {@link Cleanable} that will close the connection and release all necessary resources.
+     * Like {@link Connection#close()}, the Cleanable should no-op if called more than once.
      *
      * @param conn A {@link Connection} object.
      * @return A {@link Cleanable} instance. Calling the {@link Cleanable#close()} method will close the connection and release all resources.

core/src/main/java/org/jdbi/v3/core/Handle.java

@@ -115,12 +115,8 @@ private Handle(Jdbi jdbi,
         this.transactionHandler = transactionHandler.specialize(this);
         this.forceEndTransactions = !transactionHandler.isInTransaction(this);
 
-        addCleanable(() -> {
-            // shut down statement builder
-            if (checkConnectionIsLive()) {
-                statementBuilder.close(connection);
-            }
-        });
+        addCleanable(() ->
+            statementBuilder.close(connection));
     }
 
     /**
@@ -897,52 +893,43 @@ private void notifyHandleClosed() {
     private void cleanConnection(boolean doForceEndTransactions) {
 
         final ThrowableSuppressor throwableSuppressor = new ThrowableSuppressor();
-        final boolean connectionIsLive = checkConnectionIsLive();
 
         boolean wasInTransaction = false;
 
-        if (connectionIsLive && doForceEndTransactions) {
-            wasInTransaction = throwableSuppressor.suppressAppend(this::isInTransaction, false);
+        if (doForceEndTransactions) {
+            wasInTransaction = throwableSuppressor.suppressAppend(() -> {
+                if (connection.isClosed()) {
+                    return false;
+                }
+                return isInTransaction();
+            }, false);
         }
 
         if (wasInTransaction) {
             throwableSuppressor.suppressAppend(this::rollback);
         }
 
-        if (connectionIsLive) {
-            try {
-                connectionCleaner.close();
-            } catch (SQLException e) {
-                CloseException ce = new CloseException("Unable to close Connection", e);
-                throwableSuppressor.attachToThrowable(ce);
-                throw ce;
-            }
-
-            throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
-
-            if (wasInTransaction) {
-                TransactionException te = new TransactionException("Improper transaction handling detected: A Handle with an open "
-                        + "transaction was closed. Transactions must be explicitly committed or rolled back "
-                        + "before closing the Handle. "
-                        + "Jdbi has rolled back this transaction automatically. "
-                        + "This check may be disabled by calling getConfig(Handles.class).setForceEndTransactions(false).");
-
-                throwableSuppressor.attachToThrowable(te); // any exception present is not the cause but just collateral.
-                throw te;
-            }
-        } else {
-            throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
-        }
-    }
-
-    private boolean checkConnectionIsLive() {
         try {
-            return !connection.isClosed();
+            connectionCleaner.close();
         } catch (SQLException e) {
-            // if the connection state can not be determined, assume that the
-            // connection is closed and ignore the exception
-            return false;
+            CloseException ce = new CloseException("Unable to close Connection", e);
+            throwableSuppressor.attachToThrowable(ce);
+            throw ce;
+        }
+
+        throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
+
+        if (wasInTransaction) {
+            TransactionException te = new TransactionException("Improper transaction handling detected: A Handle with an open "
+                    + "transaction was closed. Transactions must be explicitly committed or rolled back "
+                    + "before closing the Handle. "
+                    + "Jdbi has rolled back this transaction automatically. "
+                    + "This check may be disabled by calling getConfig(Handles.class).setForceEndTransactions(false).");
+
+            throwableSuppressor.attachToThrowable(te); // any exception present is not the cause but just collateral.
+            throw te;
         }
+        throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
     }
 
     @Override

core/src/main/java/org/jdbi/v3/core/Jdbi.java

@@ -337,11 +337,13 @@ public Handler getHandler() {
      * @see #useHandle(HandleConsumer)
      * @see #withHandle(HandleCallback)
      */
+    @SuppressWarnings("PMD.CloseResource")
     public Handle open() {
+        Connection conn = null;
+        Handle h = null;
         try {
             final long start = System.nanoTime();
-            @SuppressWarnings("PMD.CloseResource")
-            Connection conn = Objects.requireNonNull(connectionFactory.openConnection(),
+            conn = Objects.requireNonNull(connectionFactory.openConnection(),
                     () -> "Connection factory " + connectionFactory + " returned a null connection");
             final long stop = System.nanoTime();
 
@@ -351,7 +353,7 @@ public Handle open() {
 
             StatementBuilder cache = statementBuilderFactory.get().createStatementBuilder(conn);
 
-            Handle h = Handle.createHandle(this,
+            h = Handle.createHandle(this,
                 connectionFactory.getCleanableFor(conn), // don't use conn::close, the cleanup must be done by the connection factory!
                 transactionhandler.get(),
                 cache,
@@ -363,6 +365,15 @@ public Handle open() {
             LOG.trace("Jdbi [{}] obtain handle [{}] in {}ms", this, h, MILLISECONDS.convert(stop - start, NANOSECONDS));
             return h;
         } catch (SQLException e) {
+            try {
+                if (h != null) {
+                    h.close();
+                } else if (conn != null) {
+                    conn.close();
+                }
+            } catch (Exception e1) {
+                e.addSuppressed(e1);
+            }
             throw new ConnectionException(e);
         }
     }