Manage in-ctor exceptions

If the c'tor of a handle throws an exception, ensure that the underlying
connection is closed as the caller will not have a chance to do so with
try-with-resources

This fixes the cause of jdbi#2446

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

@@ -111,9 +111,17 @@ private Handle(Jdbi jdbi,
         this.statementBuilder = statementBuilder;
         this.handleListeners = getConfig().get(Handles.class).copyListeners();
 
-        // both of these methods are bad because they leak a reference to this handle before the c'tor finished.
-        this.transactionHandler = transactionHandler.specialize(this);
-        this.forceEndTransactions = !transactionHandler.isInTransaction(this);
+
+        try {
+            // both of these methods are bad because they leak a reference to this handle before the c'tor finished.
+            this.transactionHandler = transactionHandler.specialize(this);
+            this.forceEndTransactions = !transactionHandler.isInTransaction(this);
+        } catch (SQLException e) {
+            // at this point, the handle is no longer viable. Close the connection
+            // and rethrow the exception
+            connectionCleaner.close();
+            throw e;
+        }
 
         addCleanable(() ->
             statementBuilder.close(connection));

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

@@ -35,6 +35,7 @@
 import org.jdbi.v3.core.internal.OnDemandExtensions;
 import org.jdbi.v3.core.internal.exceptions.Unchecked;
 import org.jdbi.v3.core.spi.JdbiPlugin;
+import org.jdbi.v3.core.statement.Cleanable;
 import org.jdbi.v3.core.statement.DefaultStatementBuilder;
 import org.jdbi.v3.core.statement.SqlStatements;
 import org.jdbi.v3.core.statement.StatementBuilder;
@@ -345,23 +346,30 @@ public Handle open() {
                     () -> "Connection factory " + connectionFactory + " returned a null connection");
             final long stop = System.nanoTime();
 
-            for (JdbiPlugin p : plugins) {
-                conn = p.customizeConnection(conn);
+            // this looks like a t-w-r but it is not. The connection is only closed in the error case.
+            final Cleanable connectionCleaner = connectionFactory.getCleanableFor(conn);
+            try {
+                for (JdbiPlugin p : plugins) {
+                    conn = p.customizeConnection(conn);
+                }
+
+                StatementBuilder cache = statementBuilderFactory.get().createStatementBuilder(conn);
+
+                Handle h = Handle.createHandle(this,
+                        connectionCleaner, // don't use conn::close, the cleanup must be done by the connection factory!
+                        transactionhandler.get(),
+                        cache,
+                        conn);
+
+                for (JdbiPlugin p : plugins) {
+                    h = p.customizeHandle(h);
+                }
+                LOG.trace("Jdbi [{}] obtain handle [{}] in {}ms", this, h, MILLISECONDS.convert(stop - start, NANOSECONDS));
+                return h;
+            } catch (Throwable t) {
+                connectionCleaner.closeAndSuppress(t);
+                throw t;
             }
-
-            StatementBuilder cache = statementBuilderFactory.get().createStatementBuilder(conn);
-
-            Handle h = Handle.createHandle(this,
-                connectionFactory.getCleanableFor(conn), // don't use conn::close, the cleanup must be done by the connection factory!
-                transactionhandler.get(),
-                cache,
-                conn);
-
-            for (JdbiPlugin p : plugins) {
-                h = p.customizeHandle(h);
-            }
-            LOG.trace("Jdbi [{}] obtain handle [{}] in {}ms", this, h, MILLISECONDS.convert(stop - start, NANOSECONDS));
-            return h;
         } catch (SQLException e) {
             throw new ConnectionException(e);
         }

core/src/main/java/org/jdbi/v3/core/statement/Cleanable.java

@@ -27,4 +27,17 @@
 public interface Cleanable extends AutoCloseable {
     @Override
     void close() throws SQLException;
+
+    /**
+     * Cleans the underlying resource and appends possible exceptions to the given {@link Throwable}.
+     *
+     * @param t A {@link Throwable}. Must not be null.
+     */
+    default void closeAndSuppress(Throwable t) {
+        try {
+            close();
+        } catch (SQLException e) {
+            t.addSuppressed(e);
+        }
+    }
 }

core/src/test/java/org/jdbi/v3/core/JdbiOpenLeakTest.java

@@ -0,0 +1,110 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jdbi.v3.core;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.jdbi.v3.core.TestHandle.ExplodeInSpecializeTransactionHandler;
+import org.jdbi.v3.core.junit5.H2DatabaseExtension;
+import org.jdbi.v3.core.spi.JdbiPlugin;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class JdbiOpenLeakTest {
+
+    @RegisterExtension
+    public H2DatabaseExtension h2Extension = H2DatabaseExtension.withSomething();
+
+    @Test
+    void cleanupCustomizeThrows() throws Exception {
+
+        var jdbiPlugin = new JdbiPlugin() {
+            @Override
+            public Connection customizeConnection(final Connection conn) throws SQLException {
+                throw new CosmicRayException();
+            }
+        };
+
+        h2Extension.getJdbi().installPlugin(jdbiPlugin);
+
+        h2Extension.clearLastConnection();
+        assertThatThrownBy(() -> h2Extension.getJdbi().open())
+                .isInstanceOf(CosmicRayException.class);
+
+        assertThat(h2Extension.getLastConnection()).isNotNull(); // has been created
+        assertThat(h2Extension.getLastConnection().isClosed()).isTrue(); // has been closed
+    }
+
+    @Test
+    void cleanupIsClosedThrows() throws Exception {
+        Connection conn = mock(Connection.class);
+        AtomicBoolean leased = new AtomicBoolean(false);
+        AtomicBoolean handleCleaned = new AtomicBoolean(false);
+
+        Jdbi jdbi = Jdbi.create(() -> {
+            leased.set(true);
+            return conn;
+        });
+
+        when(conn.isClosed()).thenThrow(CosmicRayException.class);
+        assertThat(leased.get()).isFalse();
+        assertThat(handleCleaned.get()).isFalse();
+
+        try (Handle h = jdbi.open()) {
+            h.addCleanable(() -> handleCleaned.set(true));
+        }
+
+        assertThat(handleCleaned.get()).isTrue();
+        assertThat(leased.get()).isTrue();
+        verify(conn).close();
+    }
+
+    @Test
+    public void testIssue2446() throws Exception {
+
+        h2Extension.getSharedHandle().execute("insert into something (id, name) values (1, 'Brian')");
+
+        Jdbi jdbi = h2Extension.getJdbi();
+        final ExplodeInSpecializeTransactionHandler handler = new ExplodeInSpecializeTransactionHandler();
+        h2Extension.getJdbi().setTransactionHandler(handler);
+
+        assertThatThrownBy(() -> {
+            String value;
+            h2Extension.clearLastConnection(); // reset connection
+
+            try (Handle handle = jdbi.open()) {
+                value = handle.createQuery("select name from something where id = 1").mapToBean(Something.class).one().getName();
+            }
+            assertThat(value).isEqualTo("Brian");
+        }).isInstanceOf(ConnectionException.class)
+                .hasMessageContaining("transaction specialization failure!")
+                .hasCauseInstanceOf(SQLException.class);
+
+        // see if the c'tor leaked a connection
+        assertThat(h2Extension.getLastConnection()).isNotNull(); // connection has been checked out
+        assertThat(h2Extension.getLastConnection().isClosed()).isTrue(); // connection has been closed
+    }
+
+    class CosmicRayException extends RuntimeException {
+        private static final long serialVersionUID = 1L;
+    }
+}

core/src/test/java/org/jdbi/v3/core/TestHandle.java

@@ -13,13 +13,15 @@
  */
 package org.jdbi.v3.core;
 
+import java.sql.SQLException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.jdbi.v3.core.junit5.H2DatabaseExtension;
 import org.jdbi.v3.core.statement.UnableToCreateStatementException;
 import org.jdbi.v3.core.transaction.LocalTransactionHandler;
 import org.jdbi.v3.core.transaction.TransactionException;
+import org.jdbi.v3.core.transaction.TransactionHandler;
 import org.jdbi.v3.core.transaction.TransactionIsolationLevel;
 import org.jdbi.v3.core.transaction.UnableToManipulateTransactionIsolationLevelException;
 import org.junit.jupiter.api.AfterEach;
@@ -287,4 +289,11 @@ private void maybeFail(boolean fail) {
             }
         }
     }
+
+    static class ExplodeInSpecializeTransactionHandler extends LocalTransactionHandler {
+        @Override
+        public TransactionHandler specialize(Handle handle) throws SQLException {
+            throw new SQLException("transaction specialization failure!");
+        }
+    }
 }

core/src/test/java/org/jdbi/v3/core/junit5/H2DatabaseExtension.java

@@ -13,6 +13,8 @@
  */
 package org.jdbi.v3.core.junit5;
 
+import java.sql.Connection;
+import java.sql.DriverManager;
 import java.util.LinkedHashSet;
 import java.util.Optional;
 import java.util.Set;
@@ -49,6 +51,8 @@ public final class H2DatabaseExtension implements DatabaseExtension<H2DatabaseEx
     private final boolean installPlugins;
     private Optional<DatabaseInitializer> initializerMaybe = Optional.empty();
 
+    private volatile Connection lastConnection = null;
+
     private Jdbi jdbi = null;
     private Handle sharedHandle = null;
     private boolean enableLeakchecker = true;
@@ -91,6 +95,14 @@ public Handle getSharedHandle() {
         return sharedHandle;
     }
 
+    public Connection getLastConnection() {
+        return lastConnection;
+    }
+
+    public void clearLastConnection() {
+        this.lastConnection = null;
+    }
+
     @Override
     public H2DatabaseExtension withPlugin(JdbiPlugin plugin) {
         plugins.add(plugin);
@@ -119,7 +131,10 @@ public void beforeEach(ExtensionContext context) throws Exception {
         if (jdbi != null) {
             throw new IllegalStateException("jdbi is not null!");
         }
-        jdbi = Jdbi.create(uri);
+        jdbi = Jdbi.create(() -> {
+            this.lastConnection = DriverManager.getConnection(uri);
+            return lastConnection;
+        });
 
         installTestPlugins(jdbi);