This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit c5f49fb8312ddf60386c58a7ce7dd2c2c9c3f270 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 17:30:59 2026 +0100 Commit the init SQL transaction at connection initialization Bugzilla 56586. An initSQL statement is not a health check. It is the pool deliberately initializing the physical session it has just established, and its documented purpose is to leave effects that persist for the lifetime of the connection. Commit 8926ef07a7, which replaced the commit that used to follow validation with a rollback so that validation could never commit a borrower's uncommitted work, applied that rule to both built-in validation paths and so also to the init SQL. With defaultAutoCommit off, which is the state connect() has just applied, the init statements therefore ran inside a transaction that the connect-time validation rolled back again before the first borrower ever saw the connection. The effects were not merely left uncommitted, as originally reported, but actively undone, and only non-transactional session settings escaped. validate() now ends the transaction by commit when the query it has just executed was the configured init SQL, which is exactly when the init action is running and an init SQL is configured, since that query replaces the health-check query for the action. Every other validation keeps the rollback that keeps a health check from committing application work. The commit is safe on this one action because the pool only ever runs it on a session that connect() or reconnect() established moments earlier: the previous session was closed, taking any transaction abandoned on it with it, and the only things that have run on the new one are the default state setters and the init SQL itself. A commit the driver refuses, whether it reports the failure checked or unchecked, means the session cannot be vouched for, so it fails the validation like a failed rollback does and the pool discards the connection rather than handing out a half-initialized session; the helper keeps the rule that nothing escapes validate() unchecked. The contract is now stated on PoolConfiguration.getInitSQL(), which said only that the query replaces the validation query during connection creation and was silent on what happens to the transaction it runs into. Add TestInitSqlCommittedAtInitialization, asserting against the database rather than counters that init SQL data survives to the first borrower with auto commit off and with it on, that it also survives a reconnect, that a validation query against a pool with an init SQL configured still terminates a borrower's abandoned transaction by rollback without committing anything, and that a refused commit fails initialization without throwing and stops the pool from starting. Reword the init SQL oracle in TestDefaultStateAtConnect, which pinned the previous mechanism: init SQL persistence is now guaranteed by the commit and the interceptor redundancy that test exists for is pinned by its driver call counters. --- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 14 + .../apache/tomcat/jdbc/pool/PooledConnection.java | 85 +++- .../jdbc/pool/TestDefaultStateAtConnect.java | 22 +- .../pool/TestInitSqlCommittedAtInitialization.java | 559 +++++++++++++++++++++ webapps/docs/changelog.xml | 12 + 5 files changed, 672 insertions(+), 20 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 4ff09e5b97..96db72889d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -663,6 +663,13 @@ public interface PoolConfiguration { * A custom query to be run when a connection is first created. The default value is null. This query only runs once * per connection, and that is when a new connection is established to the database. If this value is non null, it * will replace the validation query during connection creation. + * <p> + * The query runs while the pool is initializing the freshly established session, and the pool terminates the + * transaction it executes into by commit, so its effects - a data change or a session setting - persist for the + * lifetime of the connection even when the pool runs with auto commit off. A validation query follows the opposite + * rule and terminates an open transaction by rollback, so a health check never commits application work. A pool + * configured with a custom {@link Validator} does not run this query at all: the validator replaces the + * validation itself. * * @return the init SQL used to run against the DB or null if not set */ @@ -672,6 +679,13 @@ public interface PoolConfiguration { * A custom query to be run when a connection is first created. The default value is null. This query only runs once * per connection, and that is when a new connection is established to the database. If this value is non null, it * will replace the validation query during connection creation. + * <p> + * The query runs while the pool is initializing the freshly established session, and the pool terminates the + * transaction it executes into by commit, so its effects - a data change or a session setting - persist for the + * lifetime of the connection even when the pool runs with auto commit off. A validation query follows the opposite + * rule and terminates an open transaction by rollback, so a health check never commits application work. A pool + * configured with a custom {@link Validator} does not run this query at all: the validator replaces the + * validation itself. * * @param initSQL the init SQL used to run against the DB or null if no query should be executed */ diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index f04b8c3b50..1e372e0625 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -671,10 +671,14 @@ public class PooledConnection implements PooledConnectionMBean { * A validation that runs its built-in check against the database also ends any transaction that is open * on the session when the check completes - a pool configured with a custom {@link Validator} is not, * as the validator replaces the check and owns the session state it observes. The transaction ended is - * the one the check itself may have started, or one a previous borrower left open. It is ended with a - * rollback, never with a commit: the check is read-only and a pool health check must not commit - * application work the application itself never committed. When that rollback fails the session state - * is unknown, so the validation is reported as failed and the pool takes the connection out of service. + * the one the check itself may have started, or one a previous borrower left open. A read-only check is + * ended with a rollback, never with a commit: a pool health check must not commit application work the + * application itself never committed. The one exception is the {@link #VALIDATE_INIT} execution of the + * configured {@link PoolConfiguration#getInitSQL() init SQL}: that query is not a check but the pool's + * own initialization of the session it has just established, so its transaction is ended with a commit + * and the initialization persists for the connection's lifetime. When the transaction cannot be ended - + * by the commit or by the rollback - the session state is unknown, so the validation is reported as + * failed and the pool takes the connection out of service. */ public boolean validate(int validateAction, String sql) { if (connection == null) { @@ -730,7 +734,16 @@ public class PooledConnection implements PooledConnectionMBean { String query = sql; - if (validateAction == VALIDATE_INIT && poolProperties.getInitSQL() != null) { + // The init SQL is not a health check: it is the pool deliberately + // initializing the physical session it has just established, and its + // documented purpose is to leave effects that persist for the + // connection's lifetime. The pool only ever runs the init action on a + // session that connect() established moments ago, so the transaction + // this execution opens is the pool's own initialization work, and + // nothing a borrower could have left behind on it. + boolean executingInitSql = validateAction == VALIDATE_INIT && poolProperties.getInitSQL() != null; + + if (executingInitSql) { query = poolProperties.getInitSQL(); } @@ -805,13 +818,26 @@ public class PooledConnection implements PooledConnectionMBean { stmt.execute(query); stmt.close(); this.lastValidated = now; - // Terminate - by rollback, never by commit - any transaction - // the validation query opened, or that a previous borrower left - // open: a read-only health check must not commit application - // work. A session whose transaction cannot be terminated is in - // an unknown state, so it fails the validation and the pool - // takes the connection out of service. - transactionTerminated = silentlyRollbackTransactionIfNeeded(); + if (executingInitSql) { + // Terminate the transaction the init SQL executed into by + // committing it: this is the pool's own initialization of a + // freshly established session, and its effects are meant to + // persist (Bugzilla 56586). A commit the driver refuses + // leaves the session in an unknown state, so - like a failed + // rollback - it fails the validation and the pool takes the + // connection out of service, with the finally below rolling + // the session back to a clean state before it goes. + transactionTerminated = silentlyCommitTransactionIfNeeded(); + } else { + // Terminate - by rollback, never by commit - any transaction + // the validation query opened, or that a previous borrower + // left open: a read-only health check must not commit + // application work. A session whose transaction cannot be + // terminated is in an unknown state, so it fails the + // validation and the pool takes the connection out of + // service. + transactionTerminated = silentlyRollbackTransactionIfNeeded(); + } return transactionTerminated; } catch (Exception ex) { if (getPoolProperties().getLogValidationErrors()) { @@ -835,6 +861,41 @@ public class PooledConnection implements PooledConnectionMBean { } + /** + * Terminates a transaction open on the session by committing it, for the + * one case where the pending transaction is the pool's own deliberate + * initialization work: the init SQL the init-time validation has just + * executed on a freshly established session. A failure to commit - + * checked or unchecked - means the session cannot be vouched for, so it + * is reported as false and the caller treats the validation as failed; + * an unchecked failure must not escape through the caller's finally + * block, outside the catch that converts it, and strand the pooled + * connection between the idle and busy queues. + */ + private boolean silentlyCommitTransactionIfNeeded() { + try { + if (!connection.getAutoCommit()) { + connection.commit(); + } + return true; + } catch (SQLException e) { + log.debug("Failed to commit transaction", e); + } catch (RuntimeException e) { + if (getPoolProperties().getLogValidationErrors()) { + log.error("Failed to commit transaction", e); + } else { + // The unchecked variant is logged at warn rather than debug, + // like the rollback helper's: the caller surfaces it only as + // the generic validation failure, so at the default log level + // a driver that fails to commit would otherwise destroy + // connection after connection with no trace anywhere + log.warn("Failed to commit transaction", e); + } + } + return false; + } + + private boolean silentlyRollbackTransactionIfNeeded() { try { if (!connection.getAutoCommit()) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDefaultStateAtConnect.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDefaultStateAtConnect.java index d2d34e1a6d..87c087f31e 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDefaultStateAtConnect.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDefaultStateAtConnect.java @@ -50,11 +50,14 @@ import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; * inherits its job under any name. An interceptor merely named after the * state interceptor does not. * - * <p>When {@code connect()} applies {@code defaultAutoCommit=false} itself, - * the freshly executed {@code initSQL} then sits in a transaction the init-time - * validation rolls back, so the interceptor's name spelling silently decides - * whether the init SQL persists for the connection's lifetime: the last test - * pins that both spellings let it survive.</p> + * <p>The last test also pins that the {@code initSQL} persists for the + * connection's lifetime whichever spelling configures the interceptor. That + * persistence is an end-to-end invariant rather than a discriminator for the + * redundancy itself: when {@code connect()} applies + * {@code defaultAutoCommit=false} the freshly executed {@code initSQL} sits + * in a transaction that the init-time validation terminates by commit, so it + * survives even a redundant application. The driver call counts are what pin + * the redundancy away.</p> * * <p>Driver-level call counts are the measurement: the wrapping driver below * sees the direct applications from {@code connect()} (during @@ -227,9 +230,12 @@ class TestDefaultStateAtConnect extends JdbcPoolBaseTest { Assertions.assertFalse(borrower.getAutoCommit(), "the first borrower must see the configured auto commit"); } - // If connect() had applied autoCommit=false itself, the init-time - // validation would have found the init SQL in an open transaction - // and rolled it back. + // The end-to-end invariant: both name forms must hand the first + // borrower a session whose init SQL has persisted. A redundant + // application by connect() would not break this - the init-time + // validation now terminates an open transaction by commit - so + // the call counters, not this oracle, are what pin the + // redundancy away. Assertions.assertEquals(1, adminRowCount(table), "the init SQL of a pool configured as '" + interceptorConfig + "' must persist for the connection's lifetime"); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInitSqlCommittedAtInitialization.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInitSqlCommittedAtInitialization.java new file mode 100644 index 0000000000..869fdbf665 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInitSqlCommittedAtInitialization.java @@ -0,0 +1,559 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.tomcat.jdbc.pool; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; + +/** + * Expected functionality of the transaction that {@code initSQL} leaves + * open when the pool runs with auto commit off (Bugzilla 56586): the init + * SQL is not a health check but the pool's own deliberate initialization of + * a physical session it has just established, and its documented purpose is + * to leave effects that persist for the connection's lifetime. So the + * transaction it executes into must be terminated by commit at the init-time + * validation, while every genuine read-only validation - the validation + * query or {@code isValid()} at connect, borrow, return or idle - must still + * terminate any open transaction by rollback, never committing work a + * previous borrower abandoned. + * + * <p>The oracles are the committed data observed from an admin connection + * outside the pool's sessions, and the {@code commit()}/rollback() calls + * reaching the driver: an initialization that was rolled back again, and a + * health check that committed somebody's work, both fail here. A plain + * {@code SET} session statement is pinned as well - on H2 it survives a + * rollback regardless, which is why the pre-existing session-variable init + * SQL tests never noticed the missing commit.</p> + */ +class TestInitSqlCommittedAtInitialization extends JdbcPoolBaseTest { + + private static final String INIT_MARKER = "SET @POOL_INIT_STATE = 'applied'"; + + /* + * The Connection method the wrapping driver below should fail from with + * a RuntimeException, or null to let everything through. Static, because + * the driver is instantiated by class name and the tests alone decide + * when the failure window opens and closes. + */ + private static final AtomicReference<String> FAIL_FROM = new AtomicReference<>(); + + /* + * Driver-level commit()/rollback() counters. The tests are sequential + * and reset what they observe. + */ + private static final AtomicInteger COMMITS = new AtomicInteger(); + private static final AtomicInteger ROLLBACKS = new AtomicInteger(); + + private static void resetCounters() { + COMMITS.set(0); + ROLLBACKS.set(0); + FAIL_FROM.set(null); + } + + private static void resetCommitCounter() { + COMMITS.set(0); + } + + /* + * A pool on the counting driver with auto commit off and no state + * interceptor, so connect() applies the auto commit itself and the init + * SQL executes into an open transaction. The sweeper is off: these tests + * park idle connections and drive the validations themselves. + */ + private DataSource autoCommitOffPool(String initSql) { + DataSource ds = newDataSource(); + ds.setDriverClassName(InitSqlProbingDriver.class.getName()); + // a chain without the state interceptor, so connect() applies the + // auto commit state itself and the init SQL executes into an open + // transaction on every (re)connect + ds.getPoolProperties().setJdbcInterceptors(StatementFinalizer.class.getName()); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + ds.getPoolProperties().setInitSQL(initSql); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setValidationInterval(0); + return ds; + } + + private String createTable(String table) throws SQLException { + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + return table; + } + + private void createUser(String user, String grantTable) throws SQLException { + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE USER " + user + " PASSWORD 'secret'"); + st.execute("GRANT SELECT, INSERT ON " + grantTable + " TO " + user); + } + } + + private int rowCount(Connection c, String table) throws SQLException { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT COUNT(*) FROM " + table)) { + rs.next(); + return rs.getInt(1); + } + } + + private static String initMarker(Connection c) throws SQLException { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT @POOL_INIT_STATE")) { + rs.next(); + return rs.getString(1); + } + } + + /* The pooled connection behind a borrowed handle. */ + private static PooledConnection pooled(Connection borrowed) { + return interceptorOf(borrowed, ProxyConnection.class).getConnection(); + } + + @Test + void theInitSqlDmlPersistsForTheFirstBorrowerWhenAutoCommitIsOff() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + ds.createPool(); + + try (Connection borrower = ds.getConnection()) { + Assertions.assertFalse(borrower.getAutoCommit(), + "the first borrower must see the configured auto commit"); + Assertions.assertEquals(1, rowCount(borrower, table), + "the first borrower must find the initialization in effect"); + } + // Observed from outside the pool's session: the effect must be + // committed, not merely visible to the session that wrote it. + Assertions.assertEquals(1, adminRowCount(table), + "the init SQL must persist for the connection's lifetime, not be " + + "rolled back by the init-time validation"); + Assertions.assertEquals(1, COMMITS.get(), + "the pool must terminate its own initialization transaction by commit"); + } + + @Test + void theInitSqlDmlPersistsWhenAutoCommitIsOn() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + ds.createPool(); + + try (Connection borrower = ds.getConnection()) { + Assertions.assertTrue(borrower.getAutoCommit(), + "the first borrower must see the configured auto commit"); + } + Assertions.assertEquals(1, adminRowCount(table), + "the auto-committed init SQL must persist as it always did"); + Assertions.assertEquals(0, COMMITS.get(), + "with auto commit on there is nothing to commit and nothing may be rolled back"); + Assertions.assertEquals(0, ROLLBACKS.get(), + "with auto commit on there is no transaction to terminate"); + } + + @Test + void theInitSqlDmlPersistsAfterAReconnectAtBorrow() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + // A changed identity forces the borrow's reconnect fallback, which + // re-establishes the physical session without waiting on any timer; + // it is the same reconnect block a maxAge expiry drives. + ds.getPoolProperties().setAlternateUsernameAllowed(true); + createUser("re_borrower", table); + ds.createPool(); + + // The init SQL also runs on every reconnect that re-establishes the + // physical session (the credential change here); the same rule must + // hold there, or the initialization silently disappears mid-life. + Connection first = ds.getConnection(); + setSessionMarker(first, 61); + first.close(); + + try (Connection second = ds.getConnection("re_borrower", "secret")) { + Assertions.assertNull(sessionMarker(second), + "the re-borrowed session must have been replaced by a reconnect"); + } + Assertions.assertEquals(2, adminRowCount(table), + "the re-established session's init SQL must persist just like the first session's"); + } + + @Test + void aSessionSettingMadeByTheInitSqlIsVisibleToTheFirstBorrower() throws Exception { + resetCounters(); + DataSource ds = autoCommitOffPool(INIT_MARKER); + ds.createPool(); + + // H2 session variables are not transactional, so this holds whether + // or not the initialization transaction is terminated correctly - it + // is exactly why session-setting-flavoured init SQL hid the missing + // commit from the pre-existing tests. + try (Connection borrower = ds.getConnection()) { + Assertions.assertEquals("applied", initMarker(borrower), + "the init SQL must have run on the session the borrower gets"); + } + } + + @Test + void borrowValidationDoesNotCommitAbandonedWorkWhenInitSqlIsConfigured() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + ds.createPool(); + // one committed row so far: the pool's own initialization + Assertions.assertEquals(1, COMMITS.get()); + + Connection first = ds.getConnection(); + setSessionMarker(first, 51); + try (Statement st = first.createStatement()) { + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } + first.close(); // returned while still in its transaction + + resetCommitCounter(); + try (Connection second = ds.getConnection()) { + // The borrow's validation runs the validation query, not the init + // SQL: the same pool that commits its own initialization must + // still discard a previous borrower's pending work. + Assertions.assertEquals(Integer.valueOf(51), sessionMarker(second), + "the same physical session must have been reused"); + Assertions.assertEquals(1, rowCount(second, table), + "the next borrower must not find the abandoned row"); + } + Assertions.assertEquals(0, COMMITS.get(), + "the borrow validation must never commit, not even in a pool " + + "configured with init SQL"); + Assertions.assertEquals(1, adminRowCount(table), + "only the committed initialization may survive"); + } + + @Test + void theValidationQueryAtConnectStillTerminatesTheSessionTransactionByRollback() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool(null); // no init SQL: a plain health check + ds.getPoolProperties().setTestOnConnect(true); + ds.getPoolProperties().setValidationQuery("SELECT 1"); + ds.createPool(); + + Connection borrower = ds.getConnection(); + setSessionMarker(borrower, 62); + try (Statement st = borrower.createStatement()) { + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } + + // The init action with no init SQL configured is still a read-only + // health check: it must terminate the open transaction by rollback, + // never by commit. + Assertions.assertTrue(pooled(borrower).validate(PooledConnection.VALIDATE_INIT), + "the validation must pass on a live session"); + Assertions.assertEquals(0, COMMITS.get(), + "a validation query must not commit, whatever the action"); + Assertions.assertTrue(ROLLBACKS.get() > 0, + "the validation must terminate the transaction by rollback"); + + borrower.close(); + Assertions.assertEquals(0, adminRowCount(table), + "the pending work must not have survived the health check"); + } + + @Test + void aFailedCommitOfTheInitSqlIsAFailedInitializationThatDoesNotThrow() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + ds.createPool(); + try (Connection borrower = ds.getConnection()) { + Assertions.assertEquals(1, rowCount(borrower, table)); + FAIL_FROM.set("commit"); + try { + // A driver that cannot commit leaves the session in an + // unknown state: like a failed rollback, this is a failed + // validation reported as false - it must not escape from + // validate() unchecked and strand the connection. + Assertions.assertFalse(pooled(borrower).validate(PooledConnection.VALIDATE_INIT), + "a commit that the driver refuses must fail the initialization"); + } finally { + FAIL_FROM.set(null); + } + } + Assertions.assertEquals(1, adminRowCount(table), + "the init SQL execution that could not be committed must not leave its " + + "effects behind, only the one committed at creation remains"); + Assertions.assertEquals(1, ds.getPool().getSize(), + "a direct failed validation must not strand the pooled connection by itself"); + } + + @Test + void anUnreportableAutoCommitStateFailsTheInitCommitAndTheHelperOwnsIt() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + ds.createPool(); + // the initialization at creation stands committed, and it is the + // only transaction the driver was ever asked to end + Assertions.assertEquals(1, adminRowCount(table)); + Assertions.assertEquals(1, COMMITS.get()); + + try (ValidationLogCapture logs = new ValidationLogCapture()) { + Connection borrower = ds.getConnection(); + try { + FAIL_FROM.set("getAutoCommit"); + try { + // The guard that decides whether to commit must fail + // exactly like the commit it guards: a driver that + // cannot even report the transaction state yields a + // failed validation reported as false - never an + // unchecked throw escaping validate(), and never one + // that only the caller's catch happens to absorb, + // which would strand the connection wherever a caller + // terminates the transaction from a finally block + Assertions.assertFalse(pooled(borrower).validate(PooledConnection.VALIDATE_INIT), + "a transaction state the driver cannot report must fail the initialization"); + } finally { + FAIL_FROM.set(null); + } + } finally { + borrower.close(); + } + // Matching the helper's own log message is what pins which + // helper owned the failure: only the commit helper may report + // this, the armed getAutoCommit() call the rollback helper + // later makes is its own (also warning) record + logs.assertWarnedAbout("Failed to commit transaction", "boom from getAutoCommit()"); + } + Assertions.assertEquals(1, COMMITS.get(), + "the commit itself must never be reached when its guard fails"); + Assertions.assertEquals(1, adminRowCount(table), + "the init SQL that could be neither committed nor rolled back must have stayed" + + " uncommitted, leaving only the row committed at creation"); + Assertions.assertEquals(1, ds.getPool().getSize(), + "a direct failed validation must not strand the pooled connection by itself"); + } + + @Test + void aFailedCommitOfTheInitSqlStopsThePoolFromStarting() throws Exception { + resetCounters(); + String table = createTable(uniqueTable()); + DataSource ds = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + + FAIL_FROM.set("commit"); + SQLException sqlX; + try { + // The unchecked driver failure must reach the application as the + // checked SQLException a failed pool start already conveys. + sqlX = Assertions.assertThrows(SQLException.class, ds::createPool); + } finally { + FAIL_FROM.set(null); + } + Assertions.assertTrue(sqlX.getMessage().contains("Validation Query Failed"), + "expected the initialization to fail the connect validation, got: " + sqlX.getMessage()); + awaitSessions(0); + + // nothing stranded: the same pool works once the driver cooperates + DataSource working = autoCommitOffPool("INSERT INTO " + table + " VALUES (1)"); + working.createPool(); + try (Connection borrower = working.getConnection()) { + Assertions.assertEquals(1, rowCount(borrower, table)); + } + Assertions.assertEquals(1, adminRowCount(table), + "the retried initialization must persist"); + } + + /** + * Driver handing out the real H2 connections wrapped in a proxy that + * counts the {@code commit()} and {@code rollback()} calls reaching the + * driver and can fail a named method with a RuntimeException. + * Configured through {@code driverClassName}, so it must stay public + * with a no-arg constructor. + */ + public static class InitSqlProbingDriver implements Driver { + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + Connection connection = delegate.connect(url, info); + if (connection == null) { + return null; + } + return (Connection) Proxy.newProxyInstance( + InitSqlProbingDriver.class.getClassLoader(), + new Class<?>[] { Connection.class }, + new ProbingHandler(connection)); + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return delegate.acceptsURL(url); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return delegate.getPropertyInfo(url, info); + } + + @Override + public int getMajorVersion() { + return delegate.getMajorVersion(); + } + + @Override + public int getMinorVersion() { + return delegate.getMinorVersion(); + } + + @Override + public boolean jdbcCompliant() { + return delegate.jdbcCompliant(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + } + + /* Counts transaction terminations; fails the armed method by name. */ + private static class ProbingHandler implements InvocationHandler { + + private final Connection vendor; + + ProbingHandler(Connection vendor) { + this.vendor = vendor; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String armed = FAIL_FROM.get(); + if (armed != null && method.getName().equals(armed)) { + throw new IllegalStateException("boom from " + armed + "()"); + } + if ("commit".equals(method.getName())) { + COMMITS.incrementAndGet(); + } else if ("rollback".equals(method.getName())) { + ROLLBACKS.incrementAndGet(); + } + try { + return method.invoke(vendor, args); + } catch (InvocationTargetException x) { + throw x.getCause(); + } + } + } + + /* + * Captures what PooledConnection itself logs, following the + * handler-wiring approach of the pre-existing throwing-validation + * tests; the canary fails the test up front if the handler would + * observe nothing. + */ + private static final class ValidationLogCapture implements AutoCloseable { + private static final String CANARY = "pooledconnection-handler-wiring-canary"; + + private final Logger logger = Logger.getLogger(PooledConnection.class.getName()); + private final Level originalLevel = logger.getLevel(); + private final List<LogRecord> records = Collections.synchronizedList(new ArrayList<>()); + private final Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() { + // no buffering + } + + @Override + public void close() { + // nothing to release + } + }; + + ValidationLogCapture() { + logger.addHandler(handler); + logger.setLevel(Level.ALL); + LogFactory.getLog(PooledConnection.class).error(CANARY); + if (records.stream().noneMatch(r -> CANARY.equals(r.getMessage()))) { + close(); + throw new AssertionError("the test handler must observe what this class logs"); + } + // the canary is wiring proof, not pinned behavior + records.clear(); + } + + /* + * Requires a warning whose own message contains logText and whose + * attached exception carries thrownMessage. Matching the logged + * message, not just the driver's throw, is what pins which helper + * owned the failure: the commit and the rollback helper report the + * same armed call under different messages. + */ + void assertWarnedAbout(String logText, String thrownMessage) { + boolean matched = false; + synchronized (records) { + for (LogRecord record : records) { + Throwable thrown = record.getThrown(); + if (thrown != null && String.valueOf(record.getMessage()).contains(logText) + && String.valueOf(thrown.getMessage()).contains(thrownMessage)) { + Assertions.assertEquals(Level.WARNING, record.getLevel(), + "the driver failure was logged at a level the pool's default" + + " settings would hide or duplicate: " + record); + matched = true; + } + } + } + Assertions.assertTrue(matched, "expected a warning about " + logText + " carrying the" + + " driver failure (" + thrownMessage + "), records: " + records); + } + + @Override + public void close() { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 86997a0e87..143226342c 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1146,6 +1146,18 @@ connection is discarded. Restoring the transaction isolation level retains the previously logged-and-ignored behavior. (remm) </fix> + <fix> + <bug>56586</bug>: An <code>initSQL</code> statement run by the JDBC + pool while a connection is established is now committed when the pool + runs with auto commit off, so the initialization it performs - a data + change or a session setting - persists for the lifetime of the + connection. It previously ran inside a transaction that the + connect-time validation then rolled back again, so the effects were + discarded before the first borrower ever used the connection. + Validation queries and the <code>isValid()</code> check keep the + opposite rule and still terminate an open transaction with a rollback, + so a health check continues to never commit application work. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
