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 e423795473cf67ad07b9a984e1cdffdb21195079 Author: remm <[email protected]> AuthorDate: Tue Nov 3 21:25:08 2026 +0100 Make the state interceptor answer only for the borrow it serves The ConnectionState interceptor caches the tracked connection state, answers reads of it from above the borrow fence at the tail of the chain, and restores the configured auto-commit, read-only, isolation and catalog defaults on every borrow. Both halves had holes. A driver that refuses a state change while a transaction is open - MySQL (error 1568), PostgreSQL (SQLSTATE 25001) - answers with a transaction-state error (SQLSTATE class 25), and the transaction it blames may be one the terminate-first probe never saw: drivers keep reporting auto-commit true for raw-SQL work started with auto-commit on, and the restores exist to clear exactly that state. Isolation refusals used to suppress the configured defaultTransactionIsolation for the life of the session on the first such error, silently handing every later borrower the level the previous one left; the other three restores turned any class-25 refusal into a retired, healthy connection. All four refusals now share one confirmation, confirmAgainstFreedSession: the session's transaction is cleared and the restore attempted once more there. A freed session that accepts the restore serves the borrow with nothing recorded; only a refusal that repeats on a session actually freed of the state it blamed is the state's own. A confirmation rollback that fails, or a retry still refused with class 25 (the transaction will not clear), retires the session through the same IllegalStateException as the terminate-first failure: whether the previous borrower's work survived is unknown, and nothing about the refusal is confirmed. An unclearable refusal is never recorded as a permanent refusal of the value. The cache answered even when it should not. A hit never reaches the fence - that is what makes the cache worth having - so a state read parked in an interceptor above this one kept vouching for a borrow that had ended: on a session taken out of service under its borrower, where every other call failed closed, and across a clean recycle, where the re-armed pooled connection is connected and not discarded all the same and the parked read answered itself on the ended borrow's handle - in the next borrow's setup walk, with the value that borrower's restore had just pushed: a cross-borrow state disclosure. The cache now consults the same identity the tail's dispatch reads: a cached record stands only while the handle it arrived on is the handle the chain's tail is armed for, on a session still in service, and the close and abort branches seal that handle beside the session seal they already make, so the identity a borrow was served under dies with the borrow wherever it ends. A chain where a read cannot be placed on a borrow lets it fall through to the fence: a conservative fall-through is never a wrong answer. The documentation described an older, smaller interceptor. The web page called it a cache of three attributes while it tracks four, and credited it with a write-path round-trip saving it does not perform; the default* docs - web page and PoolConfiguration javadoc alike - still described connect-time application only, although connect() defers the defaults to this interceptor, which now applies them on the first borrow and re-applies what changed on every later one, with a refusal failing the borrow instead of connection creation. The class javadoc names the enforcement role, the failed-restore semantics, and the subclass obligation that had no stated contract anywhere: the pool recognises the class by type, so a reset() override that skips the superclass silently drops the enforcement. The served record's, reset()'s and the state-array contracts were corrected and condensed, and the garbled refusal log line now reads "Unable to reset the transaction isolation state on the connection." Pins: TestConnectionSetup holds the refused restore on each of the four states, the failed and unclearable confirmation rollbacks, and the replacement session's ordinary attempt; the discarded-handle pin fails the cached read where createStatement() fails; and TestCloseVsForwardedInvoke parks cached reads across close, re-borrow and an in-progress driver abort with real threads - the idle-window pin parameterized over all four cached reads, the mid-walk pin gated to the second borrow's own walk, the abort pin holding the driver call open. Every pin fails on the pre-fix code. Full mvn test of modules/jdbc-pool green (860 tests); javadoc produces no new warnings. --- modules/jdbc-pool/doc/jdbc-pool.xml | 30 +- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 39 +- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 29 +- .../jdbc/pool/interceptor/ConnectionState.java | 353 ++++++++++++---- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 +- .../jdbc/pool/TestCloseVsForwardedInvoke.java | 464 ++++++++++++++++++++- .../tomcat/jdbc/pool/TestConnectionSetup.java | 401 +++++++++++++++++- .../jdbc/pool/TestIsValidOnDiscardedHandle.java | 46 +- 8 files changed, 1248 insertions(+), 116 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index 005955e12f..d06529a6d9 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -244,11 +244,13 @@ <attributes> <attribute name="defaultAutoCommit" required="false"> - <p>(boolean) The default auto-commit state of connections created by this pool. If not set, default is JDBC driver default (If not set then the <code>setAutoCommit</code> method will not be called.)</p> + <p>(boolean) The default auto-commit state of connections created by this pool. If not set, default is JDBC driver default (If not set then the <code>setAutoCommit</code> method will not be called.) + When the <code>ConnectionState</code> interceptor is configured, it applies this setting on every borrow instead of at connection creation, and re-applies it whenever a borrower has changed it.</p> </attribute> <attribute name="defaultReadOnly" required="false"> - <p>(boolean) The default read-only state of connections created by this pool. If not set then the <code>setReadOnly</code> method will not be called. (Some drivers don't support read only mode, ex: Informix)</p> + <p>(boolean) The default read-only state of connections created by this pool. If not set then the <code>setReadOnly</code> method will not be called. (Some drivers don't support read only mode, ex: Informix) + When the <code>ConnectionState</code> interceptor is configured, it applies this setting on every borrow instead of at connection creation, and re-applies it whenever a borrower has changed it.</p> </attribute> <attribute name="defaultTransactionIsolation" required="false"> @@ -264,11 +266,11 @@ <code>2</code> for <code>READ_COMMITTED</code>, is accepted in place of the name. The name or number is matched ignoring surrounding whitespace, and any value other than the names above, their numbers or a blank value fails the creation of the pool with an error naming the attribute.</p> - <p>If not set, or set to a blank value, the method will not be called and it defaults to the JDBC driver.</p> + <p>If not set, or set to a blank value, the method will not be called and it defaults to the JDBC driver. When the <code>ConnectionState</code> interceptor is configured, it applies this setting on every borrow instead of at connection creation, and re-applies it whenever a borrower has changed it.</p> </attribute> <attribute name="defaultCatalog" required="false"> - <p>(String) The default catalog of connections created by this pool.</p> + <p>(String) The default catalog of connections created by this pool. When the <code>ConnectionState</code> interceptor is configured, it applies this setting on every borrow instead of at connection creation, and re-applies it whenever a borrower has changed it.</p> </attribute> <attribute name="driverClassName" required="true"> @@ -677,8 +679,8 @@ <subsection name="JDBC interceptors"> <p>To see an example of how to use an interceptor, take a look at <code>org.apache.tomcat.jdbc.pool.interceptor.ConnectionState</code>. - This simple interceptor is a cache of three attributes, transaction isolation level, auto commit and read only state, - in order for the system to avoid not needed roundtrips to the database. + This simple interceptor is a cache of four attributes - transaction isolation level, auto commit, read only state + and catalog - in order for the system to avoid not needed roundtrips to the database. </p> <p>Further interceptors will be added to the core of the pool as the need arises. Contributions are always welcome!</p> <p>Interceptors are of course not limited to just <code>java.sql.Connection</code> but can be used to wrap any @@ -730,7 +732,16 @@ <subsection name="org.apache.tomcat.jdbc.pool.interceptor.ConnectionState"> <p>Caches the connection for the following attributes <code>autoCommit</code>, <code>readOnly</code>, <code>transactionIsolation</code> and <code>catalog</code>. - It is a performance enhancement to avoid roundtrip to the database when getters are called or setters are called with an already set value. + It is a performance enhancement to avoid roundtrip to the database when getters are called; + setters always reach the driver, and only a successful call updates the cache. + </p> + <p>Once this interceptor is configured, the pool also leaves the enforcement of its configured defaults - + <code>defaultAutoCommit</code>, <code>defaultReadOnly</code>, <code>defaultTransactionIsolation</code> and + <code>defaultCatalog</code> - to it: they are no longer applied at connection creation, the interceptor applies + them on every borrow and restores any of them a previous borrower changed, terminating a transaction left open + by that borrower first. A restore the driver refuses for the read only, auto commit or catalog state fails the + borrow and the pool takes the connection out of service. A refused transaction isolation level is the one + exception: the connection stays in service and that level is not retried on it. </p> <attributes> </attributes> @@ -1020,8 +1031,9 @@ <p>Interceptors are a powerful way to enable, disable or modify functionality on a specific connection or its sub components. There are many different use cases for when interceptors are useful. By default, and for performance reasons, the connection pool is stateless. The only state the pool itself inserts are <code>defaultAutoCommit</code>, <code>defaultReadOnly</code>, <code>defaultTransactionIsolation</code>, <code>defaultCatalog</code> if - these are set. These 4 properties are only set upon connection creation. Should these properties be modified during the usage of the connection, - the pool itself will not reset them.</p> + these are set. Without a state interceptor these 4 properties are only set upon connection creation; should they be modified during the usage of the connection, + the pool itself will not reset them. When the <code>ConnectionState</code> interceptor is configured, it applies the 4 properties on every borrow + instead of at connection creation, and restores them whenever a borrower has changed them.</p> <p>An interceptor has to extend the <code>org.apache.tomcat.jdbc.pool.JdbcInterceptor</code> class. This class is fairly simple, You will need to have a no arg constructor</p> <source><![CDATA[ public JdbcInterceptor() { 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 9393293a62..5504693b94 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 @@ -139,6 +139,9 @@ public interface PoolConfiguration { /** * The default auto-commit state of connections created by this pool. If not set (null), default is JDBC driver * default (If set to null then the {@link java.sql.Connection#setAutoCommit(boolean)} method will not be called.) + * When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is configured, it applies + * this setting on every borrow rather than at connection creation, and re-applies it whenever a borrower has + * changed it. * * @return the default auto commit setting, null is Driver default. */ @@ -147,6 +150,9 @@ public interface PoolConfiguration { /** * The default auto-commit state of connections created by this pool. If not set (null), default is JDBC driver * default (If set to null then the {@link java.sql.Connection#setAutoCommit(boolean)} method will not be called.) + * When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is configured, it applies + * this setting on every borrow rather than at connection creation, and re-applies it whenever a borrower has + * changed it. * * @return the default auto commit setting, null is Driver default. */ @@ -155,6 +161,9 @@ public interface PoolConfiguration { /** * The default auto-commit state of connections created by this pool. If not set (null), default is JDBC driver * default (If set to null then the {@link java.sql.Connection#setAutoCommit(boolean)} method will not be called.) + * When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is configured, it applies + * this setting on every borrow rather than at connection creation, and re-applies it whenever a borrower has + * changed it. * * @param defaultAutoCommit default auto commit setting, null is Driver default. */ @@ -162,7 +171,9 @@ public interface PoolConfiguration { /** * If non null, during connection creation the method {@link java.sql.Connection#setCatalog(String)} will be called - * with the set value. + * with the set value. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is + * configured, it applies this setting on every borrow rather than at connection creation, and re-applies it + * whenever a borrower has changed it. * * @return the default catalog, null if not set and accepting the driver default. */ @@ -170,7 +181,9 @@ public interface PoolConfiguration { /** * If non null, during connection creation the method {@link java.sql.Connection#setCatalog(String)} will be called - * with the set value. + * with the set value. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is + * configured, it applies this setting on every borrow rather than at connection creation, and re-applies it + * whenever a borrower has changed it. * * @param defaultCatalog null if not set and accepting the driver default. */ @@ -178,7 +191,9 @@ public interface PoolConfiguration { /** * If non null, during connection creation the method {@link java.sql.Connection#setReadOnly(boolean)} will be - * called with the set value. + * called with the set value. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor + * is configured, it applies this setting on every borrow rather than at connection creation, and re-applies it + * whenever a borrower has changed it. * * @return null if not set and accepting the driver default otherwise the read only value */ @@ -186,7 +201,9 @@ public interface PoolConfiguration { /** * If non null, during connection creation the method {@link java.sql.Connection#setReadOnly(boolean)} will be - * called with the set value. + * called with the set value. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor + * is configured, it applies this setting on every borrow rather than at connection creation, and re-applies it + * whenever a borrower has changed it. * * @return null if not set and accepting the driver default otherwise the read only value */ @@ -194,7 +211,9 @@ public interface PoolConfiguration { /** * If non null, during connection creation the method {@link java.sql.Connection#setReadOnly(boolean)} will be - * called with the set value. + * called with the set value. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor + * is configured, it applies this setting on every borrow rather than at connection creation, and re-applies it + * whenever a borrower has changed it. * * @param defaultReadOnly null if not set and accepting the driver default. */ @@ -204,7 +223,10 @@ public interface PoolConfiguration { /** * Returns the default transaction isolation level. If set to {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} * the method {@link java.sql.Connection#setTransactionIsolation(int)} will not be called during connection - * creation. + * creation. When the {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is configured, it + * applies this setting on every borrow rather than at connection creation, and re-applies it whenever a borrower + * has changed it; a level the driver refuses on a session is the one documented exception to that enforcement - + * the session stays in service and that level is not retried on it. * * @return driver transaction isolation level, or -1 {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} if not * set. @@ -214,7 +236,10 @@ public interface PoolConfiguration { /** * If set to {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} the method * {@link java.sql.Connection#setTransactionIsolation(int)} will not be called during connection creation. Otherwise - * the method will be called with the isolation level set by this property. + * the method will be called with the isolation level set by this property. When the + * {@link org.apache.tomcat.jdbc.pool.interceptor.ConnectionState} interceptor is configured, it applies this + * setting on every borrow rather than at connection creation, and re-applies it whenever a borrower has changed it + * (see {@link #getDefaultTransactionIsolation()} for the refused-level exception). * * @param defaultTransactionIsolation a value of {@link java.sql.Connection#TRANSACTION_NONE}, * {@link java.sql.Connection#TRANSACTION_READ_COMMITTED}, diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java index f5c9859c28..5eb6099a21 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java @@ -84,8 +84,10 @@ public class ProxyConnection extends JdbcInterceptor { /** * The connection handle the pool handed to the borrow this interceptor is currently armed for, or - * <code>null</code> from {@link #reset(ConnectionPool, PooledConnection)} until - * {@link ConnectionPool#setupConnection(PooledConnection)} records the new borrow's handle. This interceptor is + * <code>null</code> once the borrow has ended: the close and abort branches of + * {@link #invoke(Object, Method, Object[])} seal it where they end a borrow, and + * {@link #reset(ConnectionPool, PooledConnection)} clears it and holds it clear until + * {@link ConnectionPool#setupConnection(PooledConnection)} records the next borrow's handle. This interceptor is * cached on the pooled connection and a clean recycle re-arms it with the same {@link PooledConnection} object * the ended borrow held, so neither the session field nor the discarded flag can tell a call of the current * borrow from one that entered the interceptor chain before the borrow ended and descends only after the @@ -128,8 +130,8 @@ public class ProxyConnection extends JdbcInterceptor { /** * Returns the handle of the borrow this interceptor is armed for. * - * @return the current borrow's handle, or <code>null</code> between a reset and the recording of the new - * borrow's handle + * @return the current borrow's handle, or <code>null</code> once that borrow has ended - its own close + * or abort, or a teardown reset - until the recording of the next borrow's handle */ public Object getBorrowHandle() { return borrowHandle; @@ -423,7 +425,8 @@ public class ProxyConnection extends JdbcInterceptor { /** * Whether the given caller is the handle of the borrow this interceptor is armed for. The read is volatile, * mirroring the reads of the session field: the recording by {@link ConnectionPool#setupConnection(PooledConnection)} - * and the clearing by {@link #reset(ConnectionPool, PooledConnection)} have no synchronization on this side. + * and the clearing by the close and abort branches of {@link #invoke(Object, Method, Object[])} and by + * {@link #reset(ConnectionPool, PooledConnection)} have no synchronization on this side. * * @param proxy the receiver the call arrived on * @@ -509,6 +512,15 @@ public class ProxyConnection extends JdbcInterceptor { // runs on the captured objects, never on the fields again; a // concurrent pool-side release counts the two paths' one terminal // step exactly once through its busy claim and released guard. + // The per-borrow identity is sealed beside the session field, not + // left to the next borrow's reset walk: a clean return runs no + // walk over this cached interceptor at all, so an unsealed handle + // would go on matching the armed identity - while the session sits + // idle, and again while the next borrow's walk is between the + // interceptors that consult it and this one's own reset - long + // enough for a call parked above the guards to be answered as the + // current borrow's when its own borrow ended here. + this.borrowHandle = null; this.connection = null; poolRef.returnConnection(poolc); return null; @@ -539,7 +551,12 @@ public class ProxyConnection extends JdbcInterceptor { // The borrow ends here, ahead of the driver call, which the // specification allows to run long: from now on every use of this // handle fails the way it fails on a closed connection, and a - // concurrent close of the same handle finds it already ended. + // concurrent close of the same handle finds it already ended. The + // identity is sealed with the session for the same reason as in + // the close branch: the guards above read it, and the promise + // this comment makes is only true for all of them once a late + // call of this handle can no longer pass as the current borrow. + this.borrowHandle = null; this.connection = null; try { if (!poolc.isDiscarded()) { diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java index d5d1b6b842..1426e09d28 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ConnectionState.java @@ -27,16 +27,28 @@ import org.apache.tomcat.jdbc.pool.DataSourceFactory; import org.apache.tomcat.jdbc.pool.JdbcInterceptor; import org.apache.tomcat.jdbc.pool.PoolConfiguration; import org.apache.tomcat.jdbc.pool.PooledConnection; +import org.apache.tomcat.jdbc.pool.ProxyConnection; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** - * Interceptor that keep track of connection state to avoid roundtrips to the database. The + * Interceptor that keeps track of connection state to avoid roundtrips to the database. The * {@link org.apache.tomcat.jdbc.pool.ConnectionPool} is optimized to do as little work as possible. The pool itself * doesn't remember settings like {@link java.sql.Connection#setAutoCommit(boolean)}, * {@link java.sql.Connection#setReadOnly(boolean)}, {@link java.sql.Connection#setCatalog(String)} or * {@link java.sql.Connection#setTransactionIsolation(int)}. It relies on the application to remember how and when these * settings have been applied. In the cases where the application code doesn't know or want to keep track of the state, * this interceptor helps cache the state, and it also avoids roundtrips to the database asking for it. + * <p> + * Caching is only half the role. Once this interceptor is configured, the pool hands it the enforcement of the four + * configured defaults - {@code defaultAutoCommit}, {@code defaultReadOnly}, {@code defaultTransactionIsolation} and + * {@code defaultCatalog}: {@link org.apache.tomcat.jdbc.pool.PooledConnection#connect()} no longer applies them at + * connection creation, and {@link #reset(ConnectionPool, PooledConnection)} applies them to every borrow and re-applies + * whatever a previous borrower changed, on the terms that method states. + * <p> + * A subclass that overrides {@link #reset(ConnectionPool, PooledConnection)} must call the superclass, or apply those + * defaults itself. The pool recognises this class by type and, having found it, leaves the defaults to the interceptor + * alone; an override that skips the superclass therefore silently drops both the connect-time application and the + * borrow-time restore, warning no one. */ public class ConnectionState extends JdbcInterceptor { @@ -50,12 +62,17 @@ public class ConnectionState extends JdbcInterceptor { } /** - * Method names for reading connection state. + * Method names for reading connection state. The index into this array and into {@link #writeState} is the + * identity of a tracked state across the class: 0 is the auto-commit state, 1 the transaction isolation level, + * 2 the read only state and 3 the catalog. The entry at an index of {@link #writeState} is the setter whose + * value the cache field of the same index holds, and {@link #invoke} matches both arrays by index against the + * cache fields. Reordering one array without the other would map a setter's value into the wrong field; adding + * a tracked state means all three - both arrays, the cache field, and the matching in {@link #invoke} - at once. */ protected final String[] readState = { "getAutoCommit", "getTransactionIsolation", "isReadOnly", "getCatalog" }; /** - * Method names for writing connection state. + * Method names for writing connection state, indexed as {@link #readState}. */ protected final String[] writeState = { "setAutoCommit", "setTransactionIsolation", "setReadOnly", "setCatalog" }; @@ -71,16 +88,11 @@ public class ConnectionState extends JdbcInterceptor { /** * The transaction isolation level this physical session refused, or - * {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} when the session has - * not refused one. A refused level does not count as needing a restore on later - * borrows, which keeps the pool out of a loop of doomed restore attempts, error - * logs and transaction terminations on every borrow; see {@link #reset}. The - * record covers the refused level only: the pool reads the configured level on - * every borrow, so a level this session has not refused - for instance one - * configured after the refusal - is restored as usual. Cleared when the session - * is replaced, so every physical session gets its own attempt. The interceptor - * instance belongs to one pooled connection, so a plain field needs no - * synchronisation. + * {@link DataSourceFactory#UNKNOWN_TRANSACTIONISOLATION} when the session has not refused one. Only confirmed + * refusals are recorded ({@link #confirmAgainstFreedSession}). The record covers the refused level only, so a + * level this session has not refused - one configured after the refusal, for instance - is still attempted, and + * a replaced session gets a fresh attempt at every level. The interceptor instance belongs to one pooled + * connection, so a plain field needs no synchronisation. */ private int refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; @@ -94,43 +106,67 @@ public class ConnectionState extends JdbcInterceptor { */ protected String catalog = null; + /** + * The pooled connection whose current borrow this instance serves, + * recorded by every {@link #reset} that arms the chain for a borrower + * and cleared when the session is taken out of service. Cached state + * reads are answered above the borrow fence at the tail of the chain - + * a hit never reaches it - so a hit may only stand while this record + * still describes a session in service: once the pool has discarded the + * session or taken it out of service, the read falls through to the + * fence and fails like every other call on the handle. Which borrower + * an in-service session belongs to is not something this record can + * see - a clean recycle hands it the same object - and that question is + * the tail's handle identity answers, so both are asked + * ({@link #chainFence}, {@link #stillServed}). + */ + private PooledConnection served = null; + + /** + * The {@link ProxyConnection} at the tail of the chain this instance serves in, resolved once on first use. + * Only the tail's identity of the handle the current borrow runs under can tell a cached read of that borrow + * from one left parked above this cache across a clean recycle ({@link #stillServed}). + */ + private ProxyConnection chainFence = null; + /** * Restores the connection state the pool was configured to enforce. The pool invokes this on every borrow, before - * the connection is handed to its next borrower. If the previous borrower returned the connection while still in a - * transaction, that transaction is terminated first: the isolation level and the read only state are transaction - * scoped on many drivers, and an open transaction would otherwise absorb the restores below (or, when auto-commit - * is restored, silently commit the previous borrower's uncommitted work). If terminating that transaction fails, - * the borrow fails and the pool retires the connection. So does a failed restore of the read-only, auto-commit or - * catalog state: a connection known to sit in a state other than the configured default must not reach the next - * borrower. A transaction isolation level the driver refuses is the exception: the connection stays in service, - * so the pool never strands borrowers over that setting. The refusal is logged once, at refusal time, and that - * level then stops counting as a pending restore on that session - later borrows neither retry it nor log it - * again nor terminate a pending transaction on its behalf. The record covers the refused level only: the pool - * reads the configured level on every borrow, so a level the session has not refused yet - one configured after - * the refusal, for instance - is attempted and applied as usual, and a replaced session retries even the same - * level. If the refused level was the only pending restore, the next borrower then inherits an open transaction - * left by the previous one, exactly as it does for a pool that configures no defaults at all. Nothing is - * terminated when there is nothing to restore - no default is configured, the - * cache already shows every configured default in place, or the only default that would need restoring is one - * this session refused - so a pool that uses this interceptor only to cache the state does not pay for the - * termination. + * the connection is handed to its next borrower. + * <p> + * When any restore is pending, a transaction the previous borrower left open is terminated first: isolation and + * read only are transaction scoped on many drivers, an open transaction would absorb the restores, and restoring + * auto-commit would silently commit the previous borrower's work. Failing to terminate that transaction, and a + * confirmed refusal to restore the read-only, auto-commit or catalog state, fail the borrow and retire the + * session - a connection known to sit in a state other than the configured one must not reach a borrower. A + * refusal the driver blames on the session's transaction state is confirmed first, against a session freed of it + * ({@link #confirmAgainstFreedSession}). + * <p> + * A transaction isolation level the driver refuses on a freed session is the one confirmed failure that does not + * retire the connection: the level is recorded ({@link #refusedIsolationLevel}) and stops counting as a pending + * restore, so later borrows neither retry it nor pay a transaction termination on its behalf. When nothing needs + * restoring, nothing is terminated - a pool that uses this interceptor only to cache the state pays nothing. * * @param parent the pool the connection belongs to, or {@code null} when the interceptor chain is being torn down * @param con the pooled connection being set up for its next borrower */ - @SuppressWarnings("null") // Suppress false positive warnings + @SuppressWarnings("null") // The resetX booleans guard each defaultX for null, but do not read to ECJ as doing so @Override public void reset(ConnectionPool parent, PooledConnection con) { if (parent == null || con == null) { - // We are resetting, reset our defaults + // Teardown walk: the chain is being taken out of service, and + // nothing may be remembered about a session that is gone. autoCommit = null; transactionIsolation = null; readOnly = null; catalog = null; refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; + served = null; return; } + // The record of the served connection travels with the arming of + // the chain and the caches, as served's javadoc states. + served = con; PoolConfiguration poolProperties = parent.getPoolProperties(); final int defaultTransactionIsolation = poolProperties.getDefaultTransactionIsolation(); final Boolean defaultReadOnly = poolProperties.getDefaultReadOnly(); @@ -145,14 +181,11 @@ public class ConnectionState extends JdbcInterceptor { (autoCommit == null || autoCommit.booleanValue() != defaultAutoCommit.booleanValue()); boolean resetCatalog = defaultCatalog != null && (catalog == null || !catalog.equals(defaultCatalog)); if (resetTransactionIsolation || resetReadOnly || resetAutoCommit || resetCatalog) { - // The previous borrower may have returned this connection while - // its transaction was still open, unless the pool was configured - // to terminate transactions on return. Transaction isolation and - // read only state are transaction scoped on many drivers (the - // change is refused or deferred until the next transaction) and - // restoring auto-commit to true would commit whatever the - // previous borrower left pending. Terminate that transaction - // first so the restores below are actually applied. + // The javadoc's terminate-first step. The transaction may have + // survived the return, unless the pool was configured to + // terminate transactions there; the probe is the same one the + // pool's own termination paths use - auto-commit off means a + // transaction is open. try { Connection raw = con.getConnection(); if (!raw.getAutoCommit()) { @@ -176,21 +209,22 @@ public class ConnectionState extends JdbcInterceptor { } catch (SQLException x) { transactionIsolation = null; // A driver that refuses a level refuses it for this session, - // and the configured level is pool configuration that changes - // rarely, so retrying the refused level per borrow would keep - // the pool paying a doomed round trip, a termination of - // whatever the previous borrower left pending, and this error - // log - on every borrow, forever. Record the refused level so - // it stops counting as a pending restore; the next borrower - // inherits an open transaction the way it does on a pool that - // configures no defaults at all. A later configuration of a - // different level, or a replaced session, clears the way for a - // fresh attempt: this record names only the refused level. - // Report it here, at refusal time, as no later borrow will log - // it again. - refusedIsolationLevel = defaultTransactionIsolation; - log.error(MessageFactory.get("connectionState.unableToResetTransactionIsolation", - defaultTransactionIsolation), x); + // and the configured level changes rarely, so retrying per + // borrow would keep the pool paying a doomed round trip and + // its error log forever - the refusal is recorded + // (refusedIsolationLevel) and reported here, at refusal + // time. A refusal reported as a transaction-state error may + // instead blame state the auto-commit probe above never saw, + // so it first gets its confirmation against a freed session. + SQLException confirmed = confirmAgainstFreedSession(con, x, + raw -> raw.setTransactionIsolation(defaultTransactionIsolation)); + if (confirmed == null) { + transactionIsolation = Integer.valueOf(defaultTransactionIsolation); + } else { + refusedIsolationLevel = defaultTransactionIsolation; + log.error(MessageFactory.get("connectionState.unableToResetTransactionIsolation", + defaultTransactionIsolation), confirmed); + } } } if (resetReadOnly) { @@ -201,13 +235,26 @@ public class ConnectionState extends JdbcInterceptor { } } catch (SQLException x) { readOnly = null; - // The connection is known to sit in a state the pool was - // configured not to hand out, and the restore that would fix - // that failed, so it must not go to the next borrower. - // ConnectionPool.setupConnection() catches this, discards the - // connection and fails the borrow. - throw new IllegalStateException( - MessageFactory.get("connectionState.unableToRestoreReadOnly", defaultReadOnly), x); + // A refusal the driver blames on the session's transaction + // state gets its confirmation against a session freed of it + // first: this very reset exists to clear exactly that state, + // and a driver that accepts the restore there was never + // unable to apply it. + SQLException confirmed = confirmAgainstFreedSession(con, x, + raw -> raw.setReadOnly(defaultReadOnly.booleanValue())); + if (confirmed == null) { + readOnly = defaultReadOnly; + } else { + // The connection is known to sit in a state the pool was + // configured not to hand out, and the restore that would + // fix that failed even on a session freed of its + // transaction, so it must not go to the next borrower. + // ConnectionPool.setupConnection() catches this, discards + // the connection and fails the borrow. + throw new IllegalStateException( + MessageFactory.get("connectionState.unableToRestoreReadOnly", defaultReadOnly), + confirmed); + } } } if (resetAutoCommit) { @@ -219,10 +266,18 @@ public class ConnectionState extends JdbcInterceptor { } catch (SQLException x) { autoCommit = null; // As for the read-only state above: the driver refused the - // configured auto-commit state, so the borrow fails and the - // connection is discarded. - throw new IllegalStateException( - MessageFactory.get("connectionState.unableToRestoreAutoCommit", defaultAutoCommit), x); + // configured auto-commit state even on a session freed of its + // transaction, so the borrow fails and the connection is + // discarded. + SQLException confirmed = confirmAgainstFreedSession(con, x, + raw -> raw.setAutoCommit(defaultAutoCommit.booleanValue())); + if (confirmed == null) { + autoCommit = defaultAutoCommit; + } else { + throw new IllegalStateException( + MessageFactory.get("connectionState.unableToRestoreAutoCommit", defaultAutoCommit), + confirmed); + } } } if (resetCatalog) { @@ -234,29 +289,175 @@ public class ConnectionState extends JdbcInterceptor { } catch (SQLException x) { catalog = null; // As for the read-only state above: the driver refused the - // configured catalog, so the borrower learns of the - // misconfiguration instead of getting a connection that was - // never pointed at the database the pool asks it to work in. - throw new IllegalStateException( - MessageFactory.get("connectionState.unableToRestoreCatalog", defaultCatalog), x); + // configured catalog even on a session freed of its + // transaction, so the borrower learns of the misconfiguration + // instead of getting a connection that was never pointed at + // the database the pool asks it to work in. + SQLException confirmed = confirmAgainstFreedSession(con, x, + raw -> raw.setCatalog(defaultCatalog)); + if (confirmed == null) { + catalog = defaultCatalog; + } else { + throw new IllegalStateException( + MessageFactory.get("connectionState.unableToRestoreCatalog", defaultCatalog), + confirmed); + } } } } + /** + * One attempt at restoring a configured connection state onto a session. + */ + private interface RestoreAttempt { + void apply(Connection raw) throws SQLException; + } + + + /** + * Confirms a restore refusal that the driver blamed on the session's + * transaction state - SQLSTATE class 25, the "not while a transaction is + * open" answer MySQL (error 1568) and PostgreSQL give a mid-transaction + * read-only, auto-commit, isolation-level or catalog change - against a + * session freed of that state, and answers with the refusal that stands. + * The probe that terminates a pending transaction ahead of the restores + * cannot see a transaction the previous borrower started with raw SQL + * while auto-commit was on, so the refusal may be about that hidden + * state rather than about the value being restored - and clearing that + * state is what the restores around this refusal exist to do. The + * session's transaction is therefore cleared with a rollback - a no-op + * the major drivers tolerate when none is open - and the restore is + * attempted once more on the freed session. + * + * @param con the pooled connection whose session produced the refusal + * @param refusal the refusal to confirm, reported while the session still + * held the state it blames + * @param retry the same restore, attempted again on the freed session + * @return {@code null} if the freed session accepted the restore; the + * exception with which the freed session refuses the restore + * for some other reason than its transaction state, confirming + * the refusal; or the original refusal, which blamed no + * transaction state and stands as reported + * @throws IllegalStateException if the confirming rollback fails, or the + * freed session answers the retry with the transaction-state + * error again - either way the transaction the refusal blamed is + * not cleared, and whether the previous borrower's work survived + * is unknown, so the connection must not go to the next borrower + * - and the refusal must not be taken as confirmed either, as it + * was never answered by a session freed of the state it blamed. + * ConnectionPool.setupConnection() catches this, discards the + * connection and fails the borrow. + */ + private SQLException confirmAgainstFreedSession(PooledConnection con, SQLException refusal, + RestoreAttempt retry) { + final String sqlState = refusal.getSQLState(); + if (sqlState == null || !sqlState.startsWith("25")) { + // The driver did not blame the session's transaction state: + // there is no state to clear and the refusal stands as reported. + return refusal; + } + Connection raw = con.getConnection(); + try { + raw.rollback(); + } catch (SQLException z) { + throw new IllegalStateException( + MessageFactory.get("connectionState.unableToTerminatePendingTransaction"), z); + } + try { + retry.apply(raw); + return null; + } catch (SQLException y) { + final String retryState = y.getSQLState(); + if (retryState != null && retryState.startsWith("25")) { + // The freed session still answers with the transaction-state + // error: the state the refusal blamed is not cleared, or the + // driver keeps reporting it. A transaction that will not + // clear is the one state no borrower may be handed, so the + // session must not stay in service - as for a confirming + // rollback that itself fails, whether the previous + // borrower's work survived is unknown. And the refusal is + // not confirmed either: what repeats is still the state's + // refusal, not the value's own, so it must not be recorded. + throw new IllegalStateException( + MessageFactory.get( + "connectionState.unableToTerminatePendingTransaction"), y); + } + // Refused a second time, now for a reason that is not the + // session's transaction state: the refusal is the value's own. + return y; + } + } + + @Override public void disconnected(ConnectionPool parent, PooledConnection con, boolean finalizing) { - // We are resetting, reset our defaults + // The session goes out of service: everything the caches describe + // is about to stop existing. autoCommit = null; transactionIsolation = null; readOnly = null; catalog = null; refusedIsolationLevel = DataSourceFactory.UNKNOWN_TRANSACTIONISOLATION; + served = null; super.disconnected(parent, con, finalizing); } + /** + * Answers whether a cached state read may stand: the session it + * describes must still be in service - not discarded under the + * borrower, its physical session not taken out of service - and the + * handle the read arrived on must still be the handle the chain's tail + * is armed for, which is the only way to tell a read of the current + * borrow from one left parked across a clean recycle that re-armed the + * very same session for the next borrower. A false answer lets the + * read fall through to the chain's tail, where the borrow fence answers + * about the handle the way it answers every other call. + * + * @param proxy the connection handle the read arrived on + * @return true if a cached state read may be answered + */ + private boolean stillServed(Object proxy) { + PooledConnection pc = served; + if (pc == null || pc.isDiscarded() || pc.getConnection() == null) { + return false; + } + ProxyConnection fence = resolveChainFence(); + return proxy != null && fence != null && proxy == fence.getBorrowHandle(); + } + + + /** + * Resolves, once, the proxy connection at the tail of the chain this + * instance serves in. The chain's wiring is fixed when it is built and + * an interceptor instance belongs to exactly one chain, so the walk + * below runs until the tail is found and then never again. A chain with + * no tail - which the pool's own setup already rules out - resolves to + * {@code null}, and the caller lets the read fall through: a + * conservative fall-through is never a wrong answer, while an answer + * the fence would have refused is. + * + * @return the borrow fence at the chain's tail, or {@code null} if + * there is none + */ + private ProxyConnection resolveChainFence() { + ProxyConnection tail = chainFence; + if (tail == null) { + JdbcInterceptor link = this; + while (link != null && !(link instanceof ProxyConnection)) { + link = link.getNext(); + } + if (link instanceof ProxyConnection) { + tail = (ProxyConnection) link; + chainFence = tail; + } + } + return tail; + } + + @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { String name = method.getName(); @@ -296,8 +497,10 @@ public class ConnectionState extends JdbcInterceptor { } default: // NOOP } - // Return cached result, if we have it - if (result != null) { + // A cached read answers only while stillServed says it may + // stand; otherwise it falls through to the chain's tail, which + // answers the handle the way it answers every other call. + if (result != null && stillServed(proxy)) { return result; } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index fb85f6978f..68e5b64c35 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -89,7 +89,7 @@ connectionPool.asyncBorrowConnectionReleased=The pooled connection served to thi connectionState.unableToRestoreAutoCommit=Unable to restore the configured auto-commit state {0} on the connection. connectionState.unableToRestoreCatalog=Unable to restore the configured default catalog {0} on the connection. connectionState.unableToRestoreReadOnly=Unable to restore the configured read-only state {0} on the connection. -connectionState.unableToResetTransactionIsolation=Unable to reset transaction isolation state to connection. The configured level {0} will not be retried on this session; it keeps its current isolation level. +connectionState.unableToResetTransactionIsolation=Unable to reset the transaction isolation state on the connection. The configured level {0} will not be retried on this session; it keeps its current isolation level. connectionState.resetStateToNull=Reset state to null as an exception occurred while calling method[{0}]. connectionState.unableToTerminatePendingTransaction=Unable to check for, or terminate, the pending transaction before restoring the configured connection state. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsForwardedInvoke.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsForwardedInvoke.java index 7664679a0e..00d60069c4 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsForwardedInvoke.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsForwardedInvoke.java @@ -34,8 +34,11 @@ import java.util.logging.Logger; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; /** * A connection-level call dispatched through a handle races a close of that @@ -261,6 +264,327 @@ class TestCloseVsForwardedInvoke extends JdbcPoolBaseTest { Assertions.assertEquals(2, pool.getReturnedCount()); } + @Test + void aParkedCachedStateReadRefusesTheReBorrowedSession() throws Exception { + // The same journey with a state read the ConnectionState interceptor + // answers from its cache: such a read never reaches the dispatch, so + // the only thing that can stand between it and a stale success is + // the same handle identity the dispatch checks. The borrow sets + // auto-commit off - a driver call that also lands in the cache, + // giving the parked read something to answer with; the borrow closes + // and a second borrower takes the session, whose reset restores the + // configured auto-commit. Letting the parked read go would answer it + // with a value only the second borrow made true, on a handle every + // other use of fails as closed. + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + ds.getPoolProperties().setJdbcInterceptors( + GateInterceptor.class.getName() + ";" + ConnectionState.class.getName()); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + Connection held = ds.getConnection(); + held.setAutoCommit(false); // reaches the driver and the interceptor cache + + // Arm the gate only for the read: the write above ran while the + // gate of a previous test stood open. + GateInterceptor.parked = new CountDownLatch(1); + GateInterceptor.gate = new CountDownLatch(1); + AtomicReference<Object> answered = new AtomicReference<>(); + AtomicReference<Throwable> readFailure = new AtomicReference<>(); + Thread reader = new Thread(() -> { + try { + answered.set(held.getAutoCommit()); + } catch (Throwable t) { + readFailure.set(t); + } + }, "parked-cached-read"); + reader.start(); + Assertions.assertTrue(GateInterceptor.parked.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the read must be parked above the state cache while the close and the re-borrow run"); + + held.close(); + Connection second = ds.getConnection(); + Assertions.assertEquals(1, pool.getActive(), + "precondition: the parked read's session now serves the second borrower"); + + GateInterceptor.gate.countDown(); + reader.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(reader.isAlive(), "the read must have been answered"); + Assertions.assertNull(answered.get(), + "a cached state read must not answer for a handle whose borrow ended -" + + " least of all with the value the next borrow's reset restored"); + Throwable failure = readFailure.get(); + Assertions.assertInstanceOf(SQLException.class, failure, + () -> "the read must fail the way a closed connection fails, not with " + failure); + Assertions.assertTrue(failure.getMessage() != null && failure.getMessage().contains("closed"), + () -> "the refusal must name the closed connection: " + failure.getMessage()); + + second.close(); + Assertions.assertEquals(2, pool.getBorrowedCount()); + Assertions.assertEquals(2, pool.getReturnedCount()); + } + + @ParameterizedTest + @ValueSource(strings = {"getAutoCommit", "isReadOnly", "getCatalog", "getTransactionIsolation"}) + void aParkedCachedStateReadRefusesTheHandleOfAnIdleClosedBorrow(String read) throws Exception { + // The window before the window above: a clean close ends the borrow + // and returns the session with no teardown walk, so the cached chain + // keeps its last borrow's arm - the closed handle included - for as + // long as the session just sits idle. The state cache answers reads + // while it believes that arm stands, and a read parked above it and + // let loose in the idle stretch would be answered from the cache for + // a handle whose every other call now fails as closed - and the gate + // it stands on is the one gate for all four tracked reads, so each + // of the four is the same violation with a different disclosure. + // The close must seal the identity the cache checks, not only the + // session. + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + ds.getPoolProperties().setJdbcInterceptors( + GateInterceptor.class.getName() + ";" + ConnectionState.class.getName()); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + Connection held = ds.getConnection(); + // Put a value for the state the parameter names into the cache - + // each set reaches the driver and is recorded from its success - + // and read it back to prove the parked read below has a cached + // value of its state to stand on. + Object recorded; + switch (read) { + case "isReadOnly": + held.setReadOnly(true); + recorded = Boolean.TRUE; + break; + case "getCatalog": + held.setCatalog("PUBLIC"); + recorded = "PUBLIC"; + break; + case "getTransactionIsolation": + held.setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + recorded = Integer.valueOf(Connection.TRANSACTION_READ_COMMITTED); + break; + default: // getAutoCommit + held.setAutoCommit(false); + recorded = Boolean.FALSE; + break; + } + Assertions.assertEquals(recorded, readState(held, read), + "precondition: the cache answers the read with the value its set left"); + + GateInterceptor.parked = new CountDownLatch(1); + GateInterceptor.gate = new CountDownLatch(1); + AtomicReference<Object> answered = new AtomicReference<>(); + AtomicReference<Throwable> readFailure = new AtomicReference<>(); + Thread reader = new Thread(() -> { + try { + answered.set(readState(held, read)); + } catch (Throwable t) { + readFailure.set(t); + } + }, "parked-idle-cached-read"); + reader.start(); + Assertions.assertTrue(GateInterceptor.parked.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the read must be parked above the state cache while the close runs"); + + // End the borrow and let it stay ended: no second borrower, no walk, + // nothing that could re-arm or reseal anything. + held.close(); + Assertions.assertEquals(0, pool.getActive()); + Assertions.assertEquals(1, pool.getIdle(), + "precondition: the returned session sits idle, chain and all"); + + GateInterceptor.gate.countDown(); + reader.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(reader.isAlive(), "the read must have been answered"); + Assertions.assertNull(answered.get(), + "a cached state read must not answer for a handle whose borrow ended," + + " least of all while its session merely sits idle behind the seal"); + Throwable failure = readFailure.get(); + Assertions.assertInstanceOf(SQLException.class, failure, + () -> "the read must fail the way a closed connection fails, not with " + failure); + Assertions.assertTrue(failure.getMessage() != null && failure.getMessage().contains("closed"), + () -> "the refusal must name the closed connection: " + failure.getMessage()); + + Assertions.assertEquals(1, pool.getBorrowedCount()); + Assertions.assertEquals(1, pool.getReturnedCount()); + } + + @Test + void aParkedCachedStateReadRefusesMidWalkOfTheNextBorrowersSetup() throws Exception { + // The window inside the next borrow's setup: the reset walk reaches + // the state cache - which re-arms and pushes the configured defaults + // to the driver - before it reaches the tail, whose reset is what + // clears the previous borrow's handle, and the new handle is recorded + // only once the walk has completed. A read parked above the cache and + // let loose inside that window meets a cache armed for the new borrow + // and a tail still armed with the old handle: the value the cache + // would answer is the one the next borrower's restore produced. The + // ended handle must be refused there too, and this drives the walk + // with real threads through the pool's own setupConnection. + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + ds.getPoolProperties().setJdbcInterceptors(GateInterceptor.class.getName() + ";" + + ConnectionState.class.getName() + ";" + WalkGateInterceptor.class.getName()); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + Connection held = ds.getConnection(); + held.setAutoCommit(false); // reaches the driver and the interceptor cache + ProxyConnection tail = interceptorOf(held, ProxyConnection.class); + + GateInterceptor.parked = new CountDownLatch(1); + GateInterceptor.gate = new CountDownLatch(1); + AtomicReference<Object> answered = new AtomicReference<>(); + AtomicReference<Throwable> readFailure = new AtomicReference<>(); + Thread reader = new Thread(() -> { + try { + answered.set(held.getAutoCommit()); + } catch (Throwable t) { + readFailure.set(t); + } + }, "parked-midwalk-cached-read"); + reader.start(); + Assertions.assertTrue(GateInterceptor.parked.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the read must be parked above the state cache while the close and the next borrow run"); + + held.close(); + + AtomicReference<Connection> secondBorrow = new AtomicReference<>(); + AtomicReference<Throwable> borrowFailure = new AtomicReference<>(); + Thread borrower = new Thread(() -> { + try { + secondBorrow.set(ds.getConnection()); + } catch (Throwable t) { + borrowFailure.set(t); + } + }, "reborrow-parked-in-setup"); + // Arm the walk gate only now: the latch is consumed by whichever + // walk reaches it first, and the first borrow's setup completed + // long ago with the gate open - so the arrival awaited below can + // only be the second borrow's walk, which is the one this test + // holds in the window between the state cache and the tail. + WalkGateInterceptor.inWalk = new CountDownLatch(1); + WalkGateInterceptor.walkGate = new CountDownLatch(1); + borrower.start(); + Assertions.assertTrue(WalkGateInterceptor.inWalk.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the next borrow's reset walk must have passed the state cache" + + " and be held short of the tail"); + Assertions.assertNull(tail.getBorrowHandle(), + "precondition: the close must have sealed the identity the interceptors read," + + " ahead of any walk - the window may no longer offer it armed"); + + GateInterceptor.gate.countDown(); + reader.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(reader.isAlive(), "the read must have been answered"); + Assertions.assertNull(answered.get(), + "a cached state read must not answer for the ended borrow mid the next" + + " borrow's setup - the cache's arm here is one the walk will never" + + " let the ended handle match legitimately"); + Throwable failure = readFailure.get(); + Assertions.assertInstanceOf(SQLException.class, failure, + () -> "the read must fail the way a closed connection fails, not with " + failure); + Assertions.assertTrue(failure.getMessage() != null && failure.getMessage().contains("closed"), + () -> "the refusal must name the closed connection: " + failure.getMessage()); + + // The walk completes and the next borrower is served normally. + WalkGateInterceptor.walkGate.countDown(); + borrower.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(borrower.isAlive(), "the second borrow must have completed"); + Assertions.assertNull(borrowFailure.get(), + () -> "the second borrow may not fail: " + borrowFailure.get()); + Connection second = secondBorrow.get(); + second.isValid(1); + second.close(); + Assertions.assertEquals(0, pool.getActive()); + Assertions.assertEquals(2, pool.getBorrowedCount()); + Assertions.assertEquals(2, pool.getReturnedCount()); + } + + @Test + void aParkedCachedStateReadRefusesWhileTheDriversAbortRuns() throws Exception { + // The abort branch seals the identity ahead of the driver call, + // which the specification allows to run long. A driver that keeps + // the abort in progress makes that stretch observable: the borrow + // has ended but the session is not yet out of service, so the only + // thing telling a read parked above the cache of this handle from + // the current borrow is the seal the abort just made. It must + // refuse while the driver is still inside the abort call - the + // promise the branch's own comment makes, kept for every guard. + BlockingAbortDriver.abortStarted = new CountDownLatch(1); + BlockingAbortDriver.releaseAbort = new CountDownLatch(1); + DataSource ds = newDataSource(); + ds.setDriverClassName(BlockingAbortDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + ds.getPoolProperties().setJdbcInterceptors( + GateInterceptor.class.getName() + ";" + ConnectionState.class.getName()); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + Connection held = ds.getConnection(); + held.setAutoCommit(false); // reaches the driver and the interceptor cache + + GateInterceptor.parked = new CountDownLatch(1); + GateInterceptor.gate = new CountDownLatch(1); + AtomicReference<Object> answered = new AtomicReference<>(); + AtomicReference<Throwable> readFailure = new AtomicReference<>(); + Thread reader = new Thread(() -> { + try { + answered.set(held.getAutoCommit()); + } catch (Throwable t) { + readFailure.set(t); + } + }, "parked-during-abort-read"); + reader.start(); + Assertions.assertTrue(GateInterceptor.parked.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the read must be parked above the state cache while the abort runs"); + + AtomicReference<Throwable> abortFailure = new AtomicReference<>(); + Thread aborer = new Thread(() -> { + try { + held.abort(Runnable::run); + } catch (Throwable t) { + abortFailure.set(t); + } + }, "aborting-the-handle"); + aborer.start(); + Assertions.assertTrue(BlockingAbortDriver.abortStarted.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "precondition: the driver must be inside the abort call"); + + GateInterceptor.gate.countDown(); + reader.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(reader.isAlive(), "the read must have been answered"); + Assertions.assertNull(answered.get(), + "a cached state read must not answer for a handle the application is" + + " aborting out of service while the driver call is still running"); + Throwable failure = readFailure.get(); + Assertions.assertInstanceOf(SQLException.class, failure, + () -> "the read must fail the way a closed connection fails, not with " + failure); + Assertions.assertTrue(failure.getMessage() != null && failure.getMessage().contains("closed"), + () -> "the refusal must name the closed connection: " + failure.getMessage()); + + // The driver's answer, however late, completes the promise the + // specification makes with it: the session is out of service and + // never re-enters the pool. + BlockingAbortDriver.releaseAbort.countDown(); + aborer.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(aborer.isAlive(), "the abort must have completed"); + Assertions.assertNull(abortFailure.get(), + () -> "the abort may not fail: " + abortFailure.get()); + Assertions.assertEquals(0, pool.getActive()); + Assertions.assertEquals(0, pool.getSize(), + "the aborted session must not go back to the pool"); + } + @Test void theReArmedChainAnswersTheEndedBorrowAsEnded() throws Throwable { // The state the parked calls of the tests above meet, reached directly @@ -405,14 +729,16 @@ class TestCloseVsForwardedInvoke extends JdbcPoolBaseTest { /* * An interceptor that parks a setAutoCommit call - and, for the accessor - * test, a getConnection call - and no other method, on a gate before - * descending into the rest of the chain, and lets every other call, the - * close among them, through. A test can thus hold a connection-level call - * above ProxyConnection's dispatch while a close of the same handle, and - * the re-borrow of its session, complete behind it. Configured through - * the jdbcInterceptors class-name list, so it must stay a public static - * class with a no-arg constructor; the gates are static fields replaced by - * each test before use, for the same reason. + * test, a getConnection call, and for the state-cache tests the four + * cached state reads - and no other method, on a gate before descending + * into the rest of the chain, and lets every other call, the close and + * the abort among them, through. A test can thus hold a connection-level + * call above ProxyConnection's dispatch - and above the caches of any + * interceptor configured beneath it - while a close or an abort of the + * same handle, and the re-borrow of its session, complete behind it. + * Configured through the jdbcInterceptors class-name list, so it must + * stay a public static class with a no-arg constructor; the gates are + * static fields replaced by each test before use, for the same reason. */ public static class GateInterceptor extends JdbcInterceptor { @@ -428,7 +754,9 @@ class TestCloseVsForwardedInvoke extends JdbcPoolBaseTest { public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { CountDownLatch currentGate = gate; String name = method.getName(); - if (currentGate != null && ("setAutoCommit".equals(name) || "getConnection".equals(name))) { + if (currentGate != null && ("setAutoCommit".equals(name) || "getAutoCommit".equals(name) + || "getConnection".equals(name) || "isReadOnly".equals(name) + || "getCatalog".equals(name) || "getTransactionIsolation".equals(name))) { parked.countDown(); currentGate.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); } @@ -436,6 +764,52 @@ class TestCloseVsForwardedInvoke extends JdbcPoolBaseTest { } } + /* + * The cached state read the parameter names, asked of the given + * connection through the full chain. + */ + private static Object readState(Connection c, String name) throws Exception { + switch (name) { + case "isReadOnly": + return Boolean.valueOf(c.isReadOnly()); + case "getCatalog": + return c.getCatalog(); + case "getTransactionIsolation": + return Integer.valueOf(c.getTransactionIsolation()); + default: // getAutoCommit + return Boolean.valueOf(c.getAutoCommit()); + } + } + + /* + * An interceptor that holds the reset walk - and only the walk, never a + * call - at its place in the chain: configured below the state cache and + * above the tail, it lets a test stop the next borrow's setup in the + * window where the cache has already re-armed but the tail has not yet + * been reset or recorded the new handle. Configured through the + * jdbcInterceptors class-name list, so it must stay a public static class + * with a no-arg constructor; the gates are static fields replaced by each + * test before use, for the same reason. + */ + public static class WalkGateInterceptor extends JdbcInterceptor { + + static volatile CountDownLatch inWalk; + static volatile CountDownLatch walkGate; + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + CountDownLatch currentGate = walkGate; + if (currentGate != null) { + inWalk.countDown(); + try { + currentGate.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException ignore) { + Thread.currentThread().interrupt(); + } + } + } + } + /* * H2 with a counter of the setAutoCommit calls that reach the driver, so a * test can tell a call the pool refused from one that ran on a session - @@ -500,4 +874,76 @@ class TestCloseVsForwardedInvoke extends JdbcPoolBaseTest { throw new SQLFeatureNotSupportedException(); } } + + /* + * H2 with an abort the test can hold in progress, so the stretch the + * abort branch of the dispatch opens ahead of the driver call - borrow + * ended, session not yet out of service - becomes observable, and a + * test can tell what a call parked above the chain meets while the + * driver is still inside it. Configured through driverClassName, so it + * must stay a public static class with a no-arg constructor; the gates + * are static fields replaced by each test before use, for the same + * reason. + */ + public static class BlockingAbortDriver implements Driver { + + static volatile CountDownLatch abortStarted; + static volatile CountDownLatch releaseAbort; + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + Connection real = delegate.connect(url, info); + if (real == null) { + return null; + } + return (Connection) Proxy.newProxyInstance( + BlockingAbortDriver.class.getClassLoader(), + new Class<?>[] { Connection.class }, + (proxy, method, args) -> { + if ("abort".equals(method.getName())) { + // A slow driver: the caller waits inside the + // call until the test lets the driver answer. + abortStarted.countDown(); + releaseAbort.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } + try { + return method.invoke(real, args); + } catch (InvocationTargetException x) { + throw x.getTargetException() != null ? x.getTargetException() : x; + } + }); + } + + @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 { + throw new SQLFeatureNotSupportedException(); + } + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java index f16ba34c02..d3af3e0c1f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestConnectionSetup.java @@ -28,6 +28,7 @@ import java.sql.SQLException; import java.sql.SQLFeatureNotSupportedException; import java.sql.Statement; import java.util.Properties; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; @@ -52,8 +53,12 @@ import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; * tolerated instead of wedging the pool, a state the driver refuses to * restore fails the borrow instead of silently handing out the wrong * state, an isolation default the driver refuses for good converges at its - * first refusal instead of looping on every borrow, and the session the - * application receives is in a defined state. + * confirmed refusal instead of looping on every borrow, one refused only for + * the session's transient state is retried and still applies, and a borrow + * whose confirmation cannot free the session from that state fails like any + * other failed transaction termination, leaving the configured level + * unsuppressed; in every case the session the application receives is in a + * defined state. * * <p>Scenarios harvested from the retired legacy classes * {@code Bug54225} (blank init SQL), {@code Bug54978} (init SQL used for @@ -82,6 +87,31 @@ class TestConnectionSetup extends JdbcPoolBaseTest { private static final AtomicInteger ROLLBACKS = new AtomicInteger(); private static final AtomicInteger ISOLATION_ATTEMPTS = new AtomicInteger(); + /* + * When positive, the wrapping proxy below refuses the next that many + * setTransactionIsolation calls with MySQL's exact SQLSTATE-class-25 + * signature of "the session is mid-transaction" and lets the later + * ones through, as the same session does once that transaction is + * cleared: a refusal of the session's state rather than of the level. + */ + private static final AtomicInteger TRANSIENT_ISOLATION_REFUSALS = new AtomicInteger(); + + /* + * When set to a state setter's name, the wrapping proxy below refuses + * calls to that method with the same class-25 signature while the + * modelled phantom transaction below is open, and lets them through + * once a rollback has cleared it: a refusal of the session's state + * rather than of the value being restored. + */ + private static final AtomicReference<String> TRANSIENT_STATE_REFUSAL = new AtomicReference<>(); + + /* + * The phantom transaction: a transaction the driver reports no other + * way than through these refusals. The terminate-first probe reads + * auto-commit, which stays true for it, so only a rollback clears it. + */ + private static final AtomicBoolean PHANTOM_TRANSACTION = new AtomicBoolean(); + private int rowCount(Connection c, String table) throws Exception { try (Statement st = c.createStatement(); ResultSet rs = st.executeQuery("SELECT COUNT(*) FROM " + table)) { @@ -280,6 +310,233 @@ class TestConnectionSetup extends JdbcPoolBaseTest { "and no borrow after the restore may terminate a transaction for it"); } + /* + * Not every refusal is the level's own. MySQL (error 1568, SQLSTATE + * 25001) and PostgreSQL refuse an isolation change while a transaction + * is open, and the session can hold one the terminate-first probe does + * not see: a transaction the previous borrower started with raw SQL + * while auto-commit was still on, which the auto-commit probe therefore + * reports as no transaction at all. Recording that one state-caused + * refusal as a refusal of the level would suppress the configured level + * for the lifetime of the physical session - every later borrower would + * silently inherit a different isolation level than the pool configures. + * The refusal is therefore confirmed - the session's transaction cleared + * and the level attempted once more - before it may suppress anything, + * which the refusal counter below pins: the transient refusal is retried + * once and the configured level ends up in effect; the level then + * converges exactly as if it had been accepted the first time, and the + * session never carries a refusal record - a session moved off the level + * is still restored to it on the next borrow. + */ + @Test + void aTransientlyRefusedIsolationRestoreIsConfirmedBeforeBeingSuppressed() throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + // H2 defaults to READ_COMMITTED, so only SERIALIZABLE can tell + // "the restore was applied" apart from "nothing happened" + ds.getPoolProperties().setDefaultTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); + ds.createPool(); + ROLLBACKS.set(0); + ISOLATION_ATTEMPTS.set(0); + TRANSIENT_ISOLATION_REFUSALS.set(1); + try { + // The first borrow: the restore hits the phantom open + // transaction and is refused; the reset clears the session's + // transaction and confirms the refusal is the level's own - + // here it is not, and the retry applies the level. + try (Connection first = ds.getConnection()) { + setSessionMarker(first, 61); + Assertions.assertEquals(Connection.TRANSACTION_SERIALIZABLE, + first.getTransactionIsolation(), + "a transiently refused restore must still deliver the " + + "configured level on the same borrow"); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "the transient refusal must be confirmed by exactly one retry"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "the confirmation must first clear the transaction the probe missed"); + + // A transient refusal left nothing recorded: a later borrow + // that finds the level in place neither re-attempts nor + // terminates anything. + try (Connection later = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(61), sessionMarker(later), + "the later borrow must reuse the same physical session"); + Assertions.assertEquals(Connection.TRANSACTION_SERIALIZABLE, + later.getTransactionIsolation(), + "the applied level must stand on later borrows"); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "an applied level must not be re-attempted on later borrows"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "and no borrow after the applied level may terminate a transaction for it"); + + // And no refusal was recorded for this session: a session moved + // off the configured level is restored to it like on a session + // that never refused anything. A recorded refusal would suppress + // this restore and leave the borrower at READ_COMMITTED. + try (Connection moved = ds.getConnection()) { + moved.setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + } + // The borrower's own write above is also a driver call; the + // restore attempts below are counted against that baseline. + int attemptsBeforeRestore = ISOLATION_ATTEMPTS.get(); + try (Connection restored = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(61), sessionMarker(restored), + "the restore must run on the same physical session"); + Assertions.assertEquals(Connection.TRANSACTION_SERIALIZABLE, + restored.getTransactionIsolation(), + "a state-caused refusal must not suppress the configured " + + "level on a later borrow"); + } + Assertions.assertEquals(attemptsBeforeRestore + 1, ISOLATION_ATTEMPTS.get(), + "the configured level must be restored after a transient refusal"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "and that restore must not terminate a transaction"); + } finally { + TRANSIENT_ISOLATION_REFUSALS.set(0); + } + } + + /* + * The confirmation only earns its record when the session actually got + * freed of the state the refusal blamed. If the clearing rollback itself + * fails, the transaction may still be open and the level was never + * attempted on a freed session: recording that as the level's own would + * suppress the configured level without any confirmation, and - worse - + * keep in service a session whose transaction state is unknown, the very + * thing the terminate-first rollback above fails the borrow for. The + * borrow must fail on that same ground, the session retire with it, and + * the never-confirmed refusal must leave no record: the replacement + * session gets its ordinary attempt of the configured level. + */ + @Test + void aFailedConfirmationRollbackFailsTheBorrowAndRetiresTheSession() throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + ds.getPoolProperties().setDefaultTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); + ds.createPool(); + ROLLBACKS.set(0); + ISOLATION_ATTEMPTS.set(0); + TRANSIENT_ISOLATION_REFUSALS.set(1); + THROW_FROM.set("rollback"); + SQLException borrowFailure; + try { + borrowFailure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a borrow whose confirmation rollback failed must not hand over a " + + "session whose transaction state is unknown"); + } finally { + THROW_FROM.set(null); + TRANSIENT_ISOLATION_REFUSALS.set(0); + } + Assertions.assertTrue(borrowFailure.getMessage().contains(ConnectionState.class.getName()), + "the borrow failure must name the failing interceptor, but was: " + borrowFailure); + Throwable terminateFailure = borrowFailure.getCause(); + Assertions.assertInstanceOf(IllegalStateException.class, terminateFailure, + "the reset must report the failed transaction termination through the " + + "pool's discard path"); + Assertions.assertInstanceOf(SQLException.class, terminateFailure.getCause(), + "the driver's rollback failure must survive as the cause"); + Assertions.assertEquals(1, ISOLATION_ATTEMPTS.get(), + "the level must not be attempted again once the transaction could not " + + "be cleared"); + + // the unknown-state session retired, not kept in service + awaitSessions(0); + Assertions.assertEquals(0, ds.getIdle()); + Assertions.assertEquals(0, ds.getSize()); + + // and the unconfirmed refusal recorded nothing: the replacement + // session is served the configured level like any first borrow + try (Connection replacement = ds.getConnection()) { + Assertions.assertEquals(Connection.TRANSACTION_SERIALIZABLE, + replacement.getTransactionIsolation(), + "a failed confirmation must not suppress the configured level on " + + "the replacement session"); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "the replacement session must get exactly its one restore attempt"); + } + + /* + * A class-25 refusal that repeats on the session freed of the state it + * blames does not confirm the level's own refusal: the freed session is + * still answering "not while a transaction is open", so what will not + * clear is exactly the transaction the refusal blamed. Recording that + * repeat as the level's permanent refusal let the borrow be served on a + * session silently holding a different level than the pool configures, + * and every later borrower inherited it while the pool believed the + * configured level refused and accepted - a level the application could + * still apply proves the record false. An unclearable transaction is + * also the one state the terminate-first rollback above never hands to a + * borrower. The repeat must therefore fail the borrow, retire the + * session, and leave the level unrecorded. + */ + @Test + void aRepeatedlyRefusedIsolationRestoreFailsTheBorrowAndRecordsNothing() throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + ds.getPoolProperties().setDefaultTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); + ds.createPool(); + ROLLBACKS.set(0); + ISOLATION_ATTEMPTS.set(0); + + // First borrow: the configured level is applied, then the + // application moves the session off it and returns it. + try (Connection first = ds.getConnection()) { + first.setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + } + Assertions.assertEquals(2, ISOLATION_ATTEMPTS.get(), + "the restore must have run once, the application's write once"); + + // Second borrow: both the restore and its retry on the session + // freed by the confirming rollback are refused with the + // transaction-state signature - the blamed state will not clear. + TRANSIENT_ISOLATION_REFUSALS.set(2); + SQLException borrowFailure; + try { + borrowFailure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a restore that is still refused as mid-transaction on a session " + + "freed of its transaction must not hand over the session"); + } finally { + TRANSIENT_ISOLATION_REFUSALS.set(0); + } + Assertions.assertTrue(borrowFailure.getMessage().contains(ConnectionState.class.getName()), + "the borrow failure must name the failing interceptor, but was: " + borrowFailure); + Throwable terminationFailure = borrowFailure.getCause(); + Assertions.assertInstanceOf(IllegalStateException.class, terminationFailure, + "the reset must report the unclearable transaction through the pool's " + + "discard path, not record the refusal"); + Assertions.assertTrue(terminationFailure.getMessage().contains("pending transaction"), + "the failure must be reported as the transaction termination it is, " + + "but was: " + terminationFailure); + Assertions.assertInstanceOf(SQLException.class, terminationFailure.getCause(), + "the repeating refusal must survive as the cause"); + Assertions.assertEquals(4, ISOLATION_ATTEMPTS.get(), + "exactly the restore and its one confirmation attempt"); + Assertions.assertEquals(1, ROLLBACKS.get(), + "the confirmation must have rolled the session back once"); + + // the unclearable-state session retired, not kept in service + awaitSessions(0); + Assertions.assertEquals(0, ds.getIdle()); + Assertions.assertEquals(0, ds.getSize()); + + // and the unconfirmed refusal recorded nothing: the replacement + // session is served the configured level like any first borrow + try (Connection replacement = ds.getConnection()) { + Assertions.assertEquals(Connection.TRANSACTION_SERIALIZABLE, + replacement.getTransactionIsolation(), + "a repeated state refusal must not suppress the configured level " + + "as a permanent one"); + } + Assertions.assertEquals(5, ISOLATION_ATTEMPTS.get(), + "the replacement session must get exactly its one restore attempt"); + } + /* * The read-only, auto-commit and catalog defaults the ConnectionState * interceptor restores on every borrow. Unlike the isolation level above, @@ -346,13 +603,120 @@ class TestConnectionSetup extends JdbcPoolBaseTest { awaitSessions(1); } + /* + * A class-25 refusal says "not while a transaction is open", and that + * transaction can be one the terminate-first probe cannot see: a + * transaction the previous borrower started with raw SQL while + * auto-commit was on, which the auto-commit probe therefore reports as + * no transaction at all. Taking such a refusal at face value - failing + * the borrow and retiring the session below - destroyed a healthy, + * correctly-restorable connection over a state that this very reset + * exists to clear: a rollback, which the major drivers tolerate even in + * auto-commit mode, frees the session and the same restore then + * succeeds. Every restore refusal that blames the session's transaction + * state is therefore confirmed against a session freed of it first; the + * rollback counter below pins the single confirming rollback, and the + * session identity that the session stays in service. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("refusedRestores") + void aTransientlyRefusedStateRestoreIsConfirmedOnAFreedSession( + String method, String value, Consumer<PoolConfiguration> configureDefault) throws Exception { + DataSource ds = newDataSource(); + ds.setDriverClassName(FailingRestoreDriver.class.getName()); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + configureDefault.accept(ds.getPoolProperties()); + ds.createPool(); + awaitSessions(1); + ROLLBACKS.set(0); + TRANSIENT_STATE_REFUSAL.set(method); + try { + // First borrow: the configured default is applied, the + // application moves the session off it, and a phantom + // transaction is left open behind the handle. + Connection first = ds.getConnection(); + setSessionMarker(first, 71); + moveOffConfiguredDefault(first, method, value); + PHANTOM_TRANSACTION.set(true); + first.close(); + Assertions.assertEquals(0, ROLLBACKS.get(), + "the terminate-first probe may not roll back: the driver " + + "reports auto-commit true for the phantom transaction"); + + // Second borrow: the restore hits the phantom transaction and + // is refused; the confirmation clears it and the freed session + // accepts the restore, so the borrow is served on the same + // session instead of failing over a state the reset clears. + try (Connection second = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(71), sessionMarker(second), + "the confirmed restore must keep the session in service"); + assertConfiguredDefault(second, method, value); + } + Assertions.assertEquals(1, ROLLBACKS.get(), + "the confirmation must clear the phantom transaction with exactly one rollback"); + Assertions.assertEquals(1, ds.getIdle(), "and the session must be back in the idle pool"); + + // The confirmed restore cached the state: a later borrow neither + // restores nor terminates anything for it. + try (Connection later = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(71), sessionMarker(later), + "the later borrow must reuse the same physical session"); + assertConfiguredDefault(later, method, value); + } + Assertions.assertEquals(1, ROLLBACKS.get(), + "an applied state must not make a later borrow terminate anything"); + } finally { + TRANSIENT_STATE_REFUSAL.set(null); + PHANTOM_TRANSACTION.set(false); + } + } + + /* Moves the session off the configured default the test exercises. */ + private static void moveOffConfiguredDefault(Connection c, String method, String value) throws SQLException { + switch (method) { + case "setReadOnly": + c.setReadOnly(!Boolean.parseBoolean(value)); + break; + case "setAutoCommit": + c.setAutoCommit(!Boolean.parseBoolean(value)); + break; + case "setCatalog": + c.setCatalog("some_other_catalog"); + break; + default: + throw new IllegalArgumentException(method); + } + } + + /* Pins that the configured default, not the moved-off one, is in effect. */ + private static void assertConfiguredDefault(Connection c, String method, String value) throws SQLException { + switch (method) { + case "setReadOnly": + Assertions.assertEquals(Boolean.parseBoolean(value), c.isReadOnly(), + "the restored read-only state must be the configured one"); + break; + case "setAutoCommit": + Assertions.assertEquals(Boolean.parseBoolean(value), c.getAutoCommit(), + "the restored auto-commit state must be the configured one"); + break; + case "setCatalog": + Assertions.assertEquals(value, c.getCatalog(), + "the restored catalog must be the configured one"); + break; + default: + throw new IllegalArgumentException(method); + } + } + /** * Driver handing out the real H2 connections wrapped in a proxy that * fails the currently armed method with a SQLException, as a real driver - * refuses a catalog it cannot select or a state it cannot apply, and - * that counts the {@code rollback()} and {@code setTransactionIsolation()} - * calls reaching the driver. Configured through {@code driverClassName}, - * so it must stay public with a no-arg constructor. + * refuses a catalog it cannot select or a state it cannot apply, refuses + * the armed number of isolation attempts as a mid-transaction refusal + * would, and that counts the {@code rollback()} and + * {@code setTransactionIsolation()} calls reaching the driver. + * Configured through {@code driverClassName}, so it must stay public + * with a no-arg constructor. */ public static class FailingRestoreDriver implements Driver { @@ -402,8 +766,11 @@ class TestConnectionSetup extends JdbcPoolBaseTest { } /* - * Fails the armed method by name and counts the rollback() and - * setTransactionIsolation() calls; delegates everything else to H2. + * Fails the armed method by name, refuses the armed number of + * setTransactionIsolation calls with a state-caused SQLSTATE 25001, + * refuses calls to the armed state setter with the same signature while + * the modelled phantom transaction is open, and counts the rollback() + * and setTransactionIsolation() calls; delegates everything else to H2. */ private static class FailingHandler implements InvocationHandler { @@ -418,10 +785,28 @@ class TestConnectionSetup extends JdbcPoolBaseTest { if (method.getName().equals(THROW_FROM.get())) { throw new SQLException("driver refuses " + method.getName() + " with these arguments"); } + if (PHANTOM_TRANSACTION.get() && method.getName().equals(TRANSIENT_STATE_REFUSAL.get())) { + throw new SQLException( + "Cannot change the session state while a transaction is in progress", + "25001", 1568); + } if ("rollback".equals(method.getName())) { ROLLBACKS.incrementAndGet(); + if (PHANTOM_TRANSACTION.getAndSet(false)) { + // The phantom transaction is a modelled driver quirk, + // not a real H2 transaction: clearing the flag is all + // this rollback has to do, and the call must not reach + // the real driver, which sits in auto-commit mode. + return null; + } } else if ("setTransactionIsolation".equals(method.getName())) { ISOLATION_ATTEMPTS.incrementAndGet(); + if (TRANSIENT_ISOLATION_REFUSALS.get() > 0) { + TRANSIENT_ISOLATION_REFUSALS.decrementAndGet(); + throw new SQLException( + "Transaction characteristics can't be changed while a transaction is in progress", + "25001", 1568); + } } try { return method.invoke(vendor, args); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestIsValidOnDiscardedHandle.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestIsValidOnDiscardedHandle.java index e5290c58a3..f8391d495a 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestIsValidOnDiscardedHandle.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestIsValidOnDiscardedHandle.java @@ -30,6 +30,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; /** * The JDBC liveness probe, {@code isValid(int)}, answers a closed @@ -44,7 +45,10 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; * reach the driver instead: the driver's own answer - including the * {@code SQLException} the specification prescribes for a negative timeout, * and including a driver-side failure of the check itself - is the - * caller's to receive, not one the pool may replace with its own. + * caller's to receive, not one the pool may replace with its own. The + * {@code ConnectionState} interceptor's cached state reads, which answer + * above the borrow fence, are pinned on the same ground: on a closed-state + * handle they must fail like every other call. */ class TestIsValidOnDiscardedHandle extends JdbcPoolBaseTest { @@ -126,8 +130,48 @@ class TestIsValidOnDiscardedHandle extends JdbcPoolBaseTest { held.close(); // and its close stays the documented no-op } + /* + * The state caches of the ConnectionState interceptor answer reads + * above the borrow fence at the chain's tail, so a cached read is the + * one answer that could keep vouching for a session the pool took out + * of service under the borrower. It must not: on a discarded handle + * every call - the cached state reads included - fails or answers + * closed alike, and only after the marking does the read fall to the + * fence. While the session is live, the reads keep being answered from + * the caches (pinned by the driver-call counters elsewhere). + */ + @Test + void aCachedStateReadOnADiscardedHandleAnswersLikeEveryOtherCall() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + ConnectionState.class.getName() + ";" + DiscardMarkingInterceptor.class.getName()); + ds.createPool(); + + Connection held = ds.getConnection(); + // The write reaches the driver and lands in the interceptor's cache. + held.setAutoCommit(false); + Assertions.assertFalse(held.getAutoCommit(), + "precondition: while the session is live the read is served, cached"); + + // The marking interceptor discards the session on the first probe, + // while the borrower still holds the handle. + Assertions.assertTrue(held.isValid(1), "precondition: the first probe is answered live"); + Assertions.assertTrue(held.isClosed(), "precondition: the marking reached the handle"); + + Assertions.assertThrows(SQLException.class, held::createStatement, + "precondition: a call that reaches the fence fails as closed"); + Assertions.assertThrows(SQLException.class, held::getAutoCommit, + "a cached state read must not vouch for a session the pool discarded" + + " under its borrower; it must fail like every other call on" + + " the handle"); + + held.close(); // the late return stays the documented no-op + awaitSessions(0); // and the marked session left with it + } + @Test void aSealedHandleKeepsAnsweringTheValidityProbeWithFalse() throws Exception { + DataSource ds = newDataSource(); ds.createPool(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
