This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 62022a82ed795c2d5b830daf79845bc9246f5962 Author: opencode <[email protected]> AuthorDate: Tue Nov 3 11:44:59 2026 +0100 Harden PooledConnection around identity, diagnostics, teardown and blank SQL Nine iterative fixes on the pool's per-connection state holder, folded into one change. Each was found in verification or clean-room review of the class and pinned by tests: TestBorrowSiteTrace and TestCloseVsConcurrentRelease are new, and TestAlternateCredentials, TestShouldForceReconnect, TestBlankValidationQuery, TestAbandonedTrace, TestAbortEndsTheBorrow, TestDriverConnectDiagnostics and TestJmxMonitor carry new pins. Identity. A pool configured without alternateUsernameAllowed no longer lets a borrow carrying its own username/password slip through: the borrow-site credentials are dropped with a once-per-pool warning (connectionPool.alternateUsernameIgnored), and shouldForceReconnect() with the flag off compares the session against the pool's own configured identity instead of answering "no change" - so a refused alternate can no longer leave the pool serving an identity that its configuration excludes. reconnectWith() is now two-phase: the new identity is put to the database while the session in hand stays in place, so a database that refuses the credentials has taken nothing out of the pool. Diagnostics. The pool masks the credentials of its configuration strings in its own toString dump; the same masking now covers the driver-instantiation debug line and the message thrown when a driver returns null for a connection, both of which printed the URL - and with it any password typed into it - in cleartext. Teardown. release() disconnects the connection before settling the released flag, and disconnect's own reentry check is defeated for as long as the first hand sits between marking the session discarded and nulling it: the abandon check and a losing return reach the same release(), and - with the per- connection lock the no-op it is by default - a JMX-invoked abandoned check makes that overlap reachable with nothing held. Each extra hand re-notified disconnected(finalizing=true) through the chain, re-closed the session and re-ran the teardown, and a connect() racing the release could re-establish behind the close. The single winner is now claimed first, under the monitor that already pairs the MBean unregistration with the flag, and only that winner acts. Abandoned traces. The borrow site lives in one slot - the unformatted frames until a report reads them, the formatted text after - published by compare-and-set on the very frames that were formatted. Formatting is deferred to the read because a pool with logAbandoned borrows constantly and reports rarely, and the atomic publication is what keeps a formatting of a previous borrower's frames from caching over the current session's site: a reader that raced a re-borrow loses the slot and the live session keeps its own borrow site. Blank SQL settings. A blank is not a statement: a configuration surface that cannot express the unset value is written blank to mean exactly that. The rule now lives in PoolProperties.configuredStatement() and is applied everywhere a SQL setting is read - both setters store through it, all three statements validate() can execute read through it (validation query, init SQL, explicit argument), and so does the pool's decision whether a new session has an init statement at all. Before that, a blank reaching the pool around a collapsing setter was executed as SQL - a pointless round trip per session, and on a driver refusing empty statements a creation that destroyed every connection - or counted as a configured statement, which sent a borrow down the init check instead of the configured borrow check and answered that check valid without touching the database. Tests and documentation. TestAbortEndsTheBorrow's abort-versus-check race closed on a returned-count-equals-rounds equality that the design never promised: when the check claims the borrow first, the application's abort is the specification's no-op and owes no hand-back; the race now asserts the witnesses the single-claim discipline does guarantee, and a deterministic test pins the no-op-after-abandon sequence. The class's javadoc lost the statements that described methods or fields which do not exist (the released flag "set to false", getExpiredAction() and the four expired methods, the weak interceptor reference, the javax.sql @see on getConnection()) and gained the notes that mark the public setters with no pool-side caller as test and embedder surface. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 39 +++- .../apache/tomcat/jdbc/pool/PoolProperties.java | 52 ++++- .../apache/tomcat/jdbc/pool/PooledConnection.java | 238 ++++++++++++--------- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../tomcat/jdbc/pool/TestAbandonedTrace.java | 33 +-- .../tomcat/jdbc/pool/TestAbortEndsTheBorrow.java | 50 ++++- .../tomcat/jdbc/pool/TestAlternateCredentials.java | 46 ++++ .../tomcat/jdbc/pool/TestBlankValidationQuery.java | 127 ++++++++++- .../tomcat/jdbc/pool/TestBorrowSiteTrace.java | 102 +++++++++ .../jdbc/pool/TestCloseVsConcurrentRelease.java | 234 ++++++++++++++++++++ ...nLog.java => TestDriverConnectDiagnostics.java} | 91 +++++++- .../tomcat/jdbc/pool/TestShouldForceReconnect.java | 19 +- .../tomcat/jdbc/pool/jmx/TestJmxMonitor.java | 10 +- 13 files changed, 885 insertions(+), 157 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 1d55dbc5c7..acf2e27f73 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -170,6 +170,11 @@ public class ConnectionPool { private final AtomicLong removeAbandonedCount = new AtomicLong(0); private final AtomicLong releasedIdleCount = new AtomicLong(0); + // Guards the one-per-pool warning that a borrow carried credentials the + // configuration will not honour, mirroring how DataSourceProxy issues the + // same warning at the application surface. + private final AtomicBoolean ignoredBorrowCredentialsWarned = new AtomicBoolean(false); + /** * Instantiate a connection pool. This will create connections if initialSize is larger than 0. The * {@link PoolProperties} should not be reused for another connection pool. @@ -1246,6 +1251,27 @@ public class ConnectionPool { throw new SQLException(MessageFactory.get("connectionPool.poolClosed")); } + if (username != null || password != null) { + if (!poolProperties.isAlternateUsernameAllowed()) { + // The credentials are not this pool's to honour. DataSourceProxy + // already ignores them at the application surface; this is the + // same contract at the pool's own, which is also reachable as + // getConnection(user,password). Honouring them here would + // authenticate a session as the borrower's identity and - since + // with alternates disallowed every later borrow is served under + // the pool's identity - hand that session to later borrowers as + // the identity they never claimed. Dropping them before the + // borrow decision below is what makes the decision's premise, + // that no session carries another identity, one the pool keeps + // rather than one it hopes for. + if (ignoredBorrowCredentialsWarned.compareAndSet(false, true)) { + log.warn(MessageFactory.get("connectionPool.alternateUsernameIgnored", username)); + } + username = null; + password = null; + } + } + // Get the current time stamp long now = System.currentTimeMillis(); // See if there is one available immediately @@ -1663,11 +1689,18 @@ public class ConnectionPool { * Returns whether new connections should be initialized by invoking {@link PooledConnection#validate(int)} with * {@link PooledConnection#VALIDATE_INIT}. * - * @return true if pool is either configured to test connections on connect or a non-NULL init SQL has been - * configured + * @return true if pool is either configured to test connections on connect or an init SQL that carries a statement + * has been configured */ private boolean isInitNewConnections() { - return getPoolProperties().isTestOnConnect() || getPoolProperties().getInitSQL() != null; + // The same reading the validation path applies to this setting: a + // blank init SQL is the unset value, so it gives a new session + // nothing to run. Counting the blank as a configured statement here + // would send a freshly re-established session down the init-check + // path - which an unset statement makes a check that runs nothing - + // instead of the borrow check the configuration did ask for. + return getPoolProperties().isTestOnConnect() || + PoolProperties.configuredStatement(getPoolProperties().getInitSQL()) != null; } /** diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index ba0920408c..5dfc98303f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -836,21 +836,16 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl /** - * Stores the validation query, treating a blank string as the unset value - * it means. A configuration surface that cannot express {@code null} - a - * JNDI attribute, a connection-properties line - is routinely written with - * an empty value to say "no statement, validate the way an unset query - * does", and a statement cannot be blank anyway; keeping such a value - * would make the pool execute the emptiness as SQL. This mirrors - * {@link #setInitSQL(String)}; a query that carries anything is stored - * verbatim. + * Stores the validation query through {@link #configuredStatement(String)}: + * a blank query is the unset value it means, not a statement. A query that + * carries anything is stored verbatim. * * @param validationQuery the query used for validation, or null or a blank * string if no validation query is set */ @Override public void setValidationQuery(String validationQuery) { - this.validationQuery = validationQuery != null && validationQuery.trim().length() > 0 ? validationQuery : null; + this.validationQuery = configuredStatement(validationQuery); } @@ -881,9 +876,19 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Stores the init SQL through {@link #configuredStatement(String)}: a + * blank init SQL is the unset value it means, not a statement a new + * session should be given. A statement that carries anything is stored + * verbatim. + * + * @param initSQL the statement a newly established session is + * initialized with, or null or a blank string if there + * is none + */ @Override public void setInitSQL(String initSQL) { - this.initSQL = initSQL != null && initSQL.trim().length() > 0 ? initSQL : null; + this.initSQL = configuredStatement(initSQL); } @@ -998,6 +1003,33 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Returns the statement an SQL setting of the configuration carries, or + * {@code null} when it carries none. + * + * <p>A blank is not a statement. A configuration surface that cannot + * express the unset value - a JNDI attribute, a connection-properties + * line - is written blank to mean exactly what the unset value means, and + * no statement can be blank. Both SQL settings the configuration keeps, + * {@link #setValidationQuery(String)} and {@link #setInitSQL(String)}, + * store through this rule, and everything that later reads them reads them + * through it as well: the statements a validation may execute, and the + * pool's decision whether a newly established session has an init + * statement to run at all. A blank that reaches the pool around a setter + * - from a {@link PoolConfiguration} implementation of the caller's own, + * or a caller's explicit validation argument - is therefore never executed + * as SQL and never counted as a configured statement.</p> + * + * @param sql the configured or passed statement, may be {@code null} + * + * @return the statement itself if it carries anything, {@code null} if it + * is null or blank + */ + static String configuredStatement(String sql) { + return sql != null && sql.trim().length() > 0 ? sql : null; + } + + /** * Masks the credentials embedded in a raw configuration string so that a * password typed into the JDBC URL or the connection properties is not diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index e9929c416b..a9a4e1c1ac 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -24,6 +24,7 @@ import java.util.HashMap; import java.util.Properties; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.management.ObjectName; @@ -90,15 +91,22 @@ public class PooledConnection implements PooledConnectionMBean { */ private volatile javax.sql.PooledConnection cpdsConnection; /** - * When we track abandon traces, this string holds the formatted thread dump - */ - private String abandonTrace = null; - /** - * When we track abandon traces, the frames of the borrow site as they were captured, until an abandoned-connection - * report reads the trace and they are formatted into {@link #abandonTrace}. A pool with logAbandoned borrows - * constantly and reports rarely, so the formatting cost is only paid when the trace is actually read. - */ - private StackTraceElement[] abandonTraceElements = null; + * When we track abandon traces, the borrow site of this connection, in one + * of three shapes: a {@link StackTraceElement} array as captured at the + * borrow and not yet formatted, the formatted trace (a String) that such an + * array was turned into, or {@code null} for no site recorded. The formatting + * is deferred to {@link #getStackTrace()} because a pool with logAbandoned + * borrows constantly and reports rarely, so the cost is only paid when the + * trace is actually read - and it is one slot rather than two fields, with + * the formatting published by compare-and-set on the very array it read, + * because the read-modify-write has no lock to sit in: the abandoned check + * that reads it can be JMX-invoked, and con.lock() is a no-op unless the + * sweeper or useLock is on. A formatting of an older borrow's frames then + * loses the slot to any re-borrow that swapped its own site in meanwhile, + * so a live session always carries its own borrow site rather than a + * previous borrower's. + */ + private final AtomicReference<Object> abandonTrace = new AtomicReference<>(); /** * Timestamp the connection was last 'touched' by the pool */ @@ -138,8 +146,10 @@ public class PooledConnection implements PooledConnectionMBean { private volatile ObjectName oname = null; /** - * Weak reference to cache the list of interceptors for this connection so that we don't create a new list of - * interceptors each time we borrow the connection + * The first link of the interceptor chain built for this connection, cached so that the chain is not rebuilt on + * every borrow. The cache holds the chain by ordinary strong references - a pooled connection keeps the interceptors + * it was given alive with itself. The pool writes the chain at each borrow (see + * {@link #setHandler(JdbcInterceptor)}) and clears it when the connection goes out of service. */ private volatile JdbcInterceptor handler = null; @@ -173,7 +183,7 @@ public class PooledConnection implements PooledConnectionMBean { * @param username The user name * @param password The password * - * @return <code>true</code>if the pool does not need to reconnect + * @return <code>true</code> if the pool does not need to reconnect */ @Deprecated public boolean checkUser(String username, String password) { @@ -181,14 +191,20 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * Returns true if we must force reconnect based on credentials passed in. Returns false if - * {@link PoolConfiguration#isAlternateUsernameAllowed()} method returns false. Returns false if the + * Returns true if we must force reconnect based on credentials passed in. Returns false if the * username/password has not changed since this connection was connected * * <p>This is a comparison only. It records nothing: the identity a session is established with is written by the * reconnect that follows this decision (see {@link #reconnectWith(String, String)}), so that a reconnect the * database refused cannot leave this connection carrying credentials that were never accepted.</p> * + * <p>Where {@link PoolConfiguration#isAlternateUsernameAllowed()} is false the passed credentials carry no + * weight - the pool ignores them at the borrow, as it does at every other surface - but the comparison is not + * skipped: with alternate identities disallowed the identity every session must serve under is the pool's own + * configured one, and a session carrying any other identity, however it came to carry it - the flag was turned + * at runtime over a session a borrower had re-established under its own credentials - is re-established rather + * than served.</p> + * * @param username the username you wish to connect with, pass in null to accept the default username from * {@link PoolConfiguration#getUsername()} * @param password the password you wish to connect with, pass in null to accept the default username from @@ -199,7 +215,15 @@ public class PooledConnection implements PooledConnectionMBean { public boolean shouldForceReconnect(String username, String password) { if (!getPoolProperties().isAlternateUsernameAllowed()) { - return false; + // Not the pool's answer to skip the comparison here: this is the + // gate that keeps a session's authenticated identity matched to + // the identity it is served under, and with alternates disallowed + // that identity is always the pool's configured one. Comparing + // against the configured credentials rather than against what the + // borrower asked for is what keeps a session that carries another + // identity from being served to a borrower that never claimed it. + username = poolProperties.getUsername(); + password = poolProperties.getPassword(); } if (username == null) { @@ -246,10 +270,6 @@ public class PooledConnection implements PooledConnectionMBean { } } } - // if (poolProperties.getDataSource()==null && poolProperties.getDataSourceJNDI()!=null) { - // TODO lookup JNDI name - // } - if (poolProperties.getDataSource() != null) { connectUsingDataSource(); } else { @@ -555,7 +575,15 @@ public class PooledConnection implements PooledConnectionMBean { } } if (connection == null) { - throw new SQLException(MessageFactory.get("pooledConnection.driverReturnedNull", driver, driverURL)); + // A driver declining the URL is the persistent-misconfiguration + // case: this message leaves connect() as the borrow's own + // SQLException and is logged as the pool's failure on every + // attempt, at the default log level. It carries the pool's own + // words, so it carries the URL masked - the way the dump and the + // debug line above do; the driver's identity and the rest of the + // URL stay readable. + throw new SQLException(MessageFactory.get("pooledConnection.driverReturnedNull", + driver, PoolProperties.maskCredentials(driverURL))); } } @@ -822,7 +850,8 @@ public class PooledConnection implements PooledConnectionMBean { return true; } else if (action == VALIDATE_INIT && poolProperties.isTestOnConnect()) { return true; - } else if (action == VALIDATE_INIT && poolProperties.getInitSQL() != null) { + } else if (action == VALIDATE_INIT && + PoolProperties.configuredStatement(poolProperties.getInitSQL()) != null) { return true; } else { return false; @@ -830,8 +859,10 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * Returns <code>true</code> if the object is still valid. if not the pool will call the getExpiredAction() and - * follow up with one of the four expired methods + * Returns <code>true</code> if the object is still valid. A false answer is the caller's cue to stop trusting this + * session: a newly established connection that fails its check is released rather than pooled, a borrow whose check + * fails is re-established before it is served, and an idle sweep that reports failure takes the connection out of + * service. * * @param validateAction The value * @@ -847,8 +878,11 @@ public class PooledConnection implements PooledConnectionMBean { * @param validateAction the action used. One of {@link #VALIDATE_BORROW}, {@link #VALIDATE_IDLE}, * {@link #VALIDATE_INIT} or {@link #VALIDATE_RETURN} * @param sql the SQL to be used during validation. If the {@link PoolConfiguration#setInitSQL(String)} - * has been called with a non null value and the action is {@link #VALIDATE_INIT} the init - * SQL will be used for validation. + * has been called with a statement and the action is {@link #VALIDATE_INIT} the init SQL + * will be used for validation, and a blank argument, like a blank configured statement, + * reads as no statement. Whether anything runs at all is the action's own configuration + * that decides: for an action the pool has not enabled, this method returns true without + * executing the statement or touching the connection. * * @return true if the connection was validated successfully. It returns true even if validation was not performed, * such as when {@link PoolConfiguration#setValidationInterval(long)} has been called with a positive @@ -924,7 +958,7 @@ public class PooledConnection implements PooledConnectionMBean { } } - String query = executableStatement(sql); + String query = PoolProperties.configuredStatement(sql); // The init SQL is not a health check: it is the pool deliberately // initializing the physical session it has just established, and its @@ -933,14 +967,15 @@ public class PooledConnection implements PooledConnectionMBean { // session that connect() established moments ago, so the transaction // this execution opens is the pool's own initialization work, and // nothing a borrower could have left behind on it. - boolean executingInitSql = validateAction == VALIDATE_INIT && poolProperties.getInitSQL() != null; + String initSql = PoolProperties.configuredStatement(poolProperties.getInitSQL()); + boolean executingInitSql = validateAction == VALIDATE_INIT && initSql != null; if (executingInitSql) { - query = poolProperties.getInitSQL(); + query = initSql; } if (query == null) { - query = executableStatement(poolProperties.getValidationQuery()); + query = PoolProperties.configuredStatement(poolProperties.getValidationQuery()); } if (query == null) { @@ -955,10 +990,7 @@ public class PooledConnection implements PooledConnectionMBean { // Terminate - by rollback, never by commit - any // transaction the isValid() call opened, or that a // previous borrower left open: a read-only health check - // must not commit application work. A session whose - // transaction cannot be terminated is in an unknown - // state, so it fails the validation and the pool takes - // the connection out of service. + // must not commit application work. transactionTerminated = silentlyRollbackTransactionIfNeeded(); return transactionTerminated; } else { @@ -1053,31 +1085,6 @@ public class PooledConnection implements PooledConnectionMBean { } - /** - * Returns the statement a validation should execute, or {@code null} when - * it has none and the built-in {@link java.sql.Connection#isValid(int)} - * check runs instead. - * - * <p>A blank string is not a statement. A configuration surface that - * cannot express the unset value - a JNDI attribute, a connection of the - * two-argument {@link #validate(int, String)} - is written blank to mean - * exactly what the unset value means, so a blank is read as unset here as - * well. {@link PoolProperties#setValidationQuery(String)} already collapses - * a blank at the write, so what reaches this point blank is a configuration - * that did not pass through that setter - one handed in as another - * {@link PoolConfiguration} implementation - or a caller's explicit - * argument, and neither should have an emptiness executed as SQL.</p> - * - * @param query the configured or passed statement, may be {@code null} - * - * @return the query itself if it carries anything, {@code null} if it is - * null or blank - */ - private static String executableStatement(String query) { - return query == null || query.trim().isEmpty() ? null : query; - } - - /** * Terminates a transaction open on the session by committing it, for the * one case where the pending transaction is the pool's own deliberate @@ -1157,21 +1164,24 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * This method is called if (Now - timeCheckedIn > getReleaseTime()) This method disconnects the connection, logs - * an error in debug mode if it happens then sets the {@link #released} flag to false. Any attempts to connect this - * cached object again will fail per {@link #connect()} The connection pool uses the atomic return value to - * decrement the pool size counter. + * This method is called if (Now - timeCheckedIn > getReleaseTime()). It disconnects the connection - a failure of + * the disconnect itself is logged at debug level - and then sets the {@link #released} flag to true, so any attempt + * to connect this cached object again fails per {@link #connect()}. Only the call that settles the flag drives the + * disconnection; later calls return without touching the session. The connection pool uses the atomic return value + * to decrement the pool size counter. * * @return true if this is the first time this method has been called. false if this method has been called before. */ public boolean release() { - try { - disconnect(true); - } catch (Exception x) { - if (log.isDebugEnabled()) { - log.debug("Unable to close SQL connection", x); - } - } + // The claim precedes the acting. release() reaches a connection from + // several hands at once - the abandon check and a losing return both + // call it on the same connection - and the terminal sequence it + // drives (the chain's disconnected(finalizing=true), the physical + // close, the chain teardown at parent.finalize) is not a sequence a + // session can be taken through twice: an interceptor would meet its + // own disposal, then the same close again. So the winner is settled + // first, under this connection's monitor, and only that winner acts. + // // Under this connection's monitor - the one lock every registration // holds from its released-check through its name assignment (see // createMBean()), and the only lock that every configuration shares, @@ -1186,38 +1196,53 @@ public class PooledConnection implements PooledConnectionMBean { // neither the name nor the flag, and publishing the orphan anyway. // With both settled here, either the registration completes first and // this unregisters it, or this passes first and the registration - // sees the released flag and publishes nothing. + // sees the released flag and publishes nothing. Settling the flag + // before the disconnect rather than after it is what stops a racing + // connect() from passing that check on a session already being torn + // down, and the MBean going before the session is what stops a JMX + // reader from watching the teardown as a live connection. + boolean first; synchronized (this) { if (oname != null) { JmxUtil.unregisterJmx(oname); oname = null; } - return released.compareAndSet(false, true); + first = released.compareAndSet(false, true); } - + if (first) { + try { + disconnect(true); + } catch (Exception x) { + if (log.isDebugEnabled()) { + log.debug("Unable to close SQL connection", x); + } + } + } + return first; } /** - * The pool will set the stack trace when it is check out and checked in + * Replaces the abandoned-connection trace. The pool itself only ever clears it - the return path hands the session + * back with a null trace so no report can read the finished borrow's site - and a non-null value is the externally + * supplied form of the trace, kept public for that use. Either way the value lands in the same slot the borrow-site + * frames live in (see {@link #abandonTrace}), so a hand-written trace is replaced by the next borrow's site just + * like a recorded one is. * - * @param trace the stack trace for this connection + * @param trace the stack trace for this connection, or null to clear it */ - public void setStackTrace(String trace) { - abandonTrace = trace; - abandonTraceElements = null; + abandonTrace.set(trace); } /** - * The pool will set the borrow-site frames when it hands the connection out; they are formatted into a trace only - * if an abandoned-connection report reads it back. The pool writes and reads the trace under the lock of this - * connection. + * Records the borrow-site frames for this connection. They are formatted into a trace only if an + * abandoned-connection report reads them back (see {@link #getStackTrace()}), and each borrow simply swaps its own + * frames into the slot, replacing whatever an earlier borrow's site had become. * * @param trace the borrow-site frames for this connection */ void setStackTraceElements(StackTraceElement[] trace) { - abandonTraceElements = trace; - abandonTrace = null; + abandonTrace.set(trace); } /** @@ -1226,20 +1251,25 @@ public class PooledConnection implements PooledConnectionMBean { * @return the stack trace or null of no trace was set */ public String getStackTrace() { - String trace = abandonTrace; - if (trace == null) { - // Format the deferred borrow site, if one is pending, and cache - // the result: the abandon path logs the trace and notifies of it, - // and both readings should pay for a single formatting - StackTraceElement[] elements = abandonTraceElements; - if (elements != null) { - Exception x = new Exception(); - x.setStackTrace(elements); - trace = ConnectionPool.getStackTrace(x); - abandonTrace = trace; - abandonTraceElements = null; - } + Object site = abandonTrace.get(); + if (site instanceof String) { + // Already formatted and cached by this or an earlier read. + return (String) site; } + StackTraceElement[] elements = (StackTraceElement[]) site; + if (elements == null) { + return null; + } + Exception x = new Exception(); + x.setStackTrace(elements); + String trace = ConnectionPool.getStackTrace(x); + // Publish through compare-and-set on the very frames that were formatted: + // a re-borrow that swapped its own site in during the formatting wins the + // slot, and this reader's text - which it still returns - was for the + // borrow that has since been replaced. (The abandonTrace field explains + // why the swap has to be atomic; caching the text is what makes the + // abandon path's two readings pay for a single formatting.) + abandonTrace.compareAndSet(elements, trace); return trace; } @@ -1288,6 +1318,10 @@ public class PooledConnection implements PooledConnectionMBean { * Set the timestamp the connection was last validated. This flag is used to keep track when we are using a * {@link PoolConfiguration#setValidationInterval(long) validation-interval}. * + * <p>The pool stamps this itself as each of its own validations completes; writing it by hand suppresses the checks + * the interval would otherwise run for up to that interval, which makes the setter a surface for tests and for an + * embedder driving that bookkeeping directly rather than a way to operate a running pool.</p> + * * @param lastValidated a timestamp as defined by {@link System#currentTimeMillis()} */ public void setLastValidated(long lastValidated) { @@ -1298,6 +1332,13 @@ public class PooledConnection implements PooledConnectionMBean { * Sets the pool configuration for this connection and connection pool. Object is shared with the * {@link ConnectionPool} * + * <p>Every connection of a pool reads the same configuration object, and the pool builds each connection against + * the one it was created with; replacing the reference here therefore changes what this single connection sees + * while the rest of the pool keeps reading the old configuration. That makes the setter a surface for tests and + * for an embedder that owns the wiring, not a way to reconfigure a running pool - which is what + * {@link DataSourceProxy#setPoolProperties(PoolConfiguration)} is, since it rebuilds the pool on the + * configuration it is given.</p> + * * @param poolProperties The pool properties */ public void setPoolProperties(PoolConfiguration poolProperties) { @@ -1375,11 +1416,12 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * Returns the underlying connection + * Returns the underlying connection: the session the pool established for this entry, not a logical handle created + * on demand. On a CPDS-configured pool the {@link javax.sql.PooledConnection} handle that creates such logical + * handles is the separate <code>cpdsConnection</code> this class keeps, just as the XA handle is + * {@link #getXaConnection()}. * * @return the underlying JDBC connection as it was returned from the JDBC driver - * - * @see javax.sql.PooledConnection#getConnection() */ public java.sql.Connection getConnection() { return this.connection; 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 a2b0ecd3bd..fdc34456d3 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 @@ -27,6 +27,7 @@ classLoaderUtil.classLoadFailed=Unable to load class: {0} from {1} classLoaderUtil.classLoadError=Unable to load class [{0}] using class loader [{1}]. classLoaderUtil.classLoadersNone=Unable to load class [{0}]: no class loaders supplied +connectionPool.alternateUsernameIgnored=alternateUsernameAllowed is set to false so the provided user name [{0}] and associated password have been ignored for this borrow connectionPool.checkAbandonedFailed=checkAbandoned failed, it will be retried. connectionPool.checkIdleFailed=checkIdle failed, it will be retried. connectionPool.connectionAbandoned=Connection has been abandoned {0}:{1} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbandonedTrace.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbandonedTrace.java index 3cbdf21eac..6bc42181a6 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbandonedTrace.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbandonedTrace.java @@ -48,16 +48,16 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; class TestAbandonedTrace extends JdbcPoolBaseTest { /* - * The trace fields are private and the point of the change is what - * they hold between the borrow and the first read, so they are read - * directly - the way the suite's other fixture reaches behind the - * pool's own accessors. + * The borrow site lives in one private slot - the unformatted frames + * until something reads them, the formatted text after - and the point + * of the change is what that slot holds between the borrow and the + * first read, so its contents are read directly - the way the suite's + * other fixture reaches behind the pool's own accessors. */ - private static Object traceField(PooledConnection pooled, String name) - throws ReflectiveOperationException { - Field field = PooledConnection.class.getDeclaredField(name); + private static Object traceSlot(PooledConnection pooled) throws ReflectiveOperationException { + Field field = PooledConnection.class.getDeclaredField("abandonTrace"); field.setAccessible(true); - return field.get(pooled); + return ((java.util.concurrent.atomic.AtomicReference<?>) field.get(pooled)).get(); } /* The pooled connection behind a borrowed handle. */ @@ -73,12 +73,13 @@ class TestAbandonedTrace extends JdbcPoolBaseTest { Connection borrowed = ds.getConnection(); try { PooledConnection pooled = pooledOf(borrowed); - Assertions.assertNull(traceField(pooled, "abandonTrace"), - "a borrow with logAbandoned must not format a thread dump up front"); - StackTraceElement[] stored = - (StackTraceElement[]) traceField(pooled, "abandonTraceElements"); - Assertions.assertNotNull(stored, - "the borrow must record the borrow site, unformatted"); + Object site = traceSlot(pooled); + Assertions.assertTrue(site instanceof StackTraceElement[], + "a borrow with logAbandoned must record the borrow site as unformatted frames, " + + "not a thread dump up front, but the slot held: " + + (site == null ? "null" : site.getClass().getName())); + Assertions.assertTrue(((StackTraceElement[]) site).length > 0, + "the borrow must record a borrow site"); String trace = pooled.getStackTrace(); Assertions.assertTrue(trace.startsWith("java.lang.Exception"), @@ -94,8 +95,8 @@ class TestAbandonedTrace extends JdbcPoolBaseTest { Assertions.assertEquals(trace, pooled.getStackTrace()); Assertions.assertSame(trace, pooled.getStackTrace(), "the formatted trace must be cached, not reformatted per reading"); - Assertions.assertNull(traceField(pooled, "abandonTraceElements"), - "the elements must not be kept once they have been formatted"); + Assertions.assertSame(trace, traceSlot(pooled), + "the slot now holds the formatted text, and no longer the frames"); } finally { borrowed.close(); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java index 44bdd972c5..2c475f4508 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java @@ -380,6 +380,39 @@ class TestAbortEndsTheBorrow extends JdbcPoolBaseTest { } } + @Test + void anAbortAfterTheCheckEndedTheBorrowIsTheSpecificationsNoOp() throws Exception { + // The sequential form of the round below where the watchdog wins the + // claim: the check ends the borrow, and the application - unaware - + // aborts the handle it still holds. The abort is then what the + // specification prescribes for a closed connection, a no-op, and this + // test pins what that costs the bookkeeping: no hand-back is counted + // for a borrow whose end the pool already paid for. + SpecAbortDriver.ABORTS.set(0); + DataSource ds = abortPool(SpecAbortDriver.class, 1); + ds.getPoolProperties().setRemoveAbandoned(true); + ds.getPoolProperties().setRemoveAbandonedTimeout(1); + ConnectionPool pool = ds.getPool(); + + Connection overdue = ds.getConnection(); + Thread.sleep(1100); + + pool.checkAbandoned(); + Assertions.assertEquals(1, pool.getRemoveAbandonedCount(), "the check claimed and abandoned the borrow"); + Assertions.assertEquals(1, pool.getReleasedCount(), "exactly one terminal release settled it"); + Assertions.assertTrue(overdue.isClosed(), "the handle of an abandoned borrow reports itself closed"); + + Assertions.assertDoesNotThrow(() -> overdue.abort(DIRECT_EXECUTOR), + "aborting a handle the pool already ended is the specification's no-op"); + Assertions.assertEquals(0, SpecAbortDriver.ABORTS.get(), "the no-op does not reach the driver"); + Assertions.assertEquals(0, pool.getActive()); + Assertions.assertEquals(0, pool.getSize()); + Assertions.assertEquals(1, pool.getReleasedCount(), "the abort added no second terminal step"); + Assertions.assertEquals(0, pool.getReturnedCount(), + "and nothing was handed back for a borrow the check had already ended"); + awaitSessions(0); + } + @Test void abortingABorrowThatTheAbandonedCheckAlsoClaimsEndsItExactlyOnce() throws Exception { // The abandoned-connection check and the borrower's abort are two @@ -448,8 +481,21 @@ class TestAbortEndsTheBorrow extends JdbcPoolBaseTest { awaitSessions(0); // and the physical session really left the database } Assertions.assertEquals(rounds, pool.getBorrowedCount()); - Assertions.assertEquals(rounds, pool.getReturnedCount(), - "every borrow ended exactly once, whichever of the two steps won each race"); + // The single-shot witness across the two claimants is the terminal + // release, not the hand-back count. The released guard settles exactly + // one release per borrow whichever step claimed it, while a hand-back + // is counted only when the abort reached the return: an abort that + // arrived after the check's teardown is the no-op the test above + // pins, and owes no count. Every round did end - the per-round + // assertions ruled the stranded shapes out, and each borrow below was + // either handed back or abandoned. + Assertions.assertEquals(rounds, pool.getReleasedCount(), + "every borrow was settled by exactly one terminal release, whichever of the two steps won each race"); + Assertions.assertTrue(pool.getReturnedCount() + pool.getRemoveAbandonedCount() >= rounds, + () -> "every borrow was either handed back or abandoned, got returned=" + + pool.getReturnedCount() + " abandoned=" + pool.getRemoveAbandonedCount()); + Assertions.assertTrue(pool.getReturnedCount() <= rounds, + "no borrow was handed back more than once"); } @Test diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java index 1a73e3876e..1ba60e3db4 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java @@ -133,6 +133,52 @@ class TestAlternateCredentials extends JdbcPoolBaseTest { awaitSessions(1); } + @Test + void credentialsHandedToThePoolSurfaceAreIgnoredLikeEverywhereElse() throws Exception { + DataSource ds = alternatePool(false); + // No pre-created session to hand out: this borrow must be the one to + // create the physical session, which is exactly where credentials + // used to enter a pool that disallows alternates. + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + + // The application surface (DataSourceProxy) ignores credentials when + // alternates are disallowed; the pool's own getConnection(user,password) + // must not be a door behind it. Honouring them there would authenticate + // a session as the claimant and then serve that session, under the + // pool's own identity, to every borrower afterwards. + try (Connection c = ds.getPool().getConnection(ALT_USER, ALT_PASSWORD)) { + Assertions.assertEquals(DEFAULT_USER, sessionUser(c), + "the ignored credentials must not authenticate the session"); + } + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(DEFAULT_USER, sessionUser(c), + "and the pool keeps serving under its own identity afterwards"); + } + } + + @Test + void aSessionCarryingAnotherIdentityIsReEstablishedWhenAlternatesAreTurnedOff() throws Exception { + DataSource ds = alternatePool(true); + ds.createPool(); + + Connection alt = ds.getConnection(ALT_USER, ALT_PASSWORD); + setSessionMarker(alt, 33); + alt.close(); + + // An operator turns the feature off while an alternate session is + // alive in the pool. Nothing can purge every queue at that instant, + // but the session must never reach a default borrower as it stands. + ds.getPoolProperties().setAlternateUsernameAllowed(false); + + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(DEFAULT_USER, sessionUser(c), + "the identity gate must not itself be switched off by the flag"); + Assertions.assertNull(sessionMarker(c), + "the pool's own identity means a session the alternate never touched"); + } + } + @Test void aWrongAlternatePasswordIsRefusedAndThePoolSurvives() throws Exception { DataSource ds = alternatePool(true); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java index db50a9efcc..b6a77c9e67 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBlankValidationQuery.java @@ -37,7 +37,10 @@ import org.junit.jupiter.params.provider.ValueSource; * reads a blank as unset even when it reaches the configuration around that * setter - through a {@link PoolConfiguration} implementation of the pool's * own, or as the explicit statement argument of - * {@link PooledConnection#validate(int, String)}. + * {@link PooledConnection#validate(int, String)}. The init SQL the pool runs + * on a session it has just established is a statement of the same path and + * reads the same way: a blank one means there is nothing to initialize the + * session with, not an emptiness to execute. * * <p>The path witness is a recording {@link Connection}: the built-in check * goes through {@code isValid()} and the statement path goes through @@ -128,26 +131,33 @@ class TestBlankValidationQuery { return null; } - /** A configuration handing out a query that never passed through the collapsing setter. */ + /** A configuration handing out statements that never passed through the collapsing setters. */ private static final class BypassedConfiguration extends PoolProperties { private final String rawQuery; + private final String rawInitSql; - BypassedConfiguration(String rawQuery) { + BypassedConfiguration(String rawQuery, String rawInitSql) { this.rawQuery = rawQuery; + this.rawInitSql = rawInitSql; } @Override public String getValidationQuery() { return rawQuery; } + + @Override + public String getInitSQL() { + return rawInitSql; + } } /** - * A connection that is up against a stand-in {@link javax.sql.DataSource} - * handing out the recording connection - the connect genuinely runs, but - * every call the validation makes lands in the recorder. + * Applies the stand-in plumbing around a recording session and brings the + * pool up on it. Whatever the pool reads while it starts - the initial + * size, JMX - must already be set on the configuration when this runs. */ - private static PooledConnection connectedTo(PoolProperties props, Recording rec) throws Exception { + private static ConnectionPool standInPool(PoolProperties props, Recording rec) throws Exception { props.setDataSource(Proxy.newProxyInstance(TestBlankValidationQuery.class.getClassLoader(), new Class<?>[] { javax.sql.DataSource.class }, (proxy, method, args) -> { switch (method.getName()) { @@ -164,14 +174,18 @@ class TestBlankValidationQuery { } })); props.setJmxEnabled(false); - props.setInitialSize(0); props.setTimeBetweenEvictionRunsMillis(-1); // Without this the interval suppression - by design - answers "true" // for the first seconds after construction and the validation never // reaches the calls this recorder watches. props.setValidationInterval(0); + return new ConnectionPool(props); + } + + private static PooledConnection connectedTo(PoolProperties props, Recording rec) throws Exception { + props.setInitialSize(0); props.setTestOnBorrow(true); - PooledConnection con = new PooledConnection(props, new ConnectionPool(props)); + PooledConnection con = new PooledConnection(props, standInPool(props, rec)); con.connect(); return con; } @@ -182,7 +196,7 @@ class TestBlankValidationQuery { Recording rec = new Recording(); // A PoolConfiguration implementation of the caller's own can hand out a // blank the pool's setter would never have stored. - PooledConnection con = connectedTo(new BypassedConfiguration(blank), rec); + PooledConnection con = connectedTo(new BypassedConfiguration(blank, null), rec); Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_BORROW), "a blank query means the built-in check, which the session passes"); @@ -217,6 +231,47 @@ class TestBlankValidationQuery { "a caller that names a statement gets that statement, not the built-in check"); } + @ParameterizedTest(name = "init SQL [{0}] is not executed as a statement") + @ValueSource(strings = { "", " ", "\t" }) + void aBlankInitSqlReachingTheValidationAroundTheSetterIsNotExecuted(String blank) throws Exception { + Recording rec = new Recording(); + PooledConnection con = connectedTo(new BypassedConfiguration(null, blank), rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_INIT), + "a blank init SQL means the unset value, and a session with nothing to run needs no check"); + Assertions.assertEquals(0, rec.executeCalls, + "an emptiness must not be executed as the session's initialization"); + Assertions.assertEquals(0, rec.isValidCalls, + "with the init statement unset and no test on connect configured, there is nothing to run"); + } + + @Test + void aBlankInitSqlLeavesTheBuiltInCheckToRunWhenConnectTestsTheSession() throws Exception { + Recording rec = new Recording(); + BypassedConfiguration props = new BypassedConfiguration(null, " "); + props.setTestOnConnect(true); + PooledConnection con = connectedTo(props, rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_INIT), + "the built-in check the unset statement means, against a session that is up"); + Assertions.assertEquals(1, rec.isValidCalls, "the built-in check is what the init action ran"); + Assertions.assertEquals(0, rec.executeCalls, "and no emptiness alongside it"); + } + + @Test + void aConfiguredInitSqlIsStillExecutedOnConnect() throws Exception { + Recording rec = new Recording(); + PoolProperties props = new PoolProperties(); + props.setInitSQL("SET SESSION MARK 7"); + PooledConnection con = connectedTo(props, rec); + + Assertions.assertTrue(con.validate(PooledConnection.VALIDATE_INIT)); + Assertions.assertEquals(1, rec.executeCalls); + Assertions.assertEquals("SET SESSION MARK 7", rec.lastExecuted, + "an init SQL that carries something is still the session's initialization"); + Assertions.assertEquals(0, rec.isValidCalls, "a named init statement is what runs, not the built-in check"); + } + @ParameterizedTest(name = "setter collapses [{0}] to unset") @ValueSource(strings = { "", " ", "\t" }) void theSetterStoresABlankQueryAsTheUnsetValue(String blank) { @@ -234,4 +289,56 @@ class TestBlankValidationQuery { Assertions.assertEquals(" SELECT 1 FROM DUAL ", props.getValidationQuery(), "a query that carries anything is stored exactly as written"); } + + @Test + void aBorrowThatReconnectsStillRunsTheConfiguredBorrowCheck() throws Exception { + Recording rec = new Recording(); + BypassedConfiguration props = new BypassedConfiguration("SELECT 1", " "); + props.setTestOnBorrow(true); + props.setInitialSize(1); + // The session goes stale the moment the pool makes it, so the borrow + // below takes the reconnect path - the path on which the pool decides + // between the init check and the borrow check by whether an init + // statement is configured. A blank one must not read as configured. + props.setMaxAge(1); + ConnectionPool pool = standInPool(props, rec); + Thread.sleep(5); + int executedBefore = rec.executeCalls; + + // The witness is read before the connection goes back: at a maxAge of + // a millisecond the return path reconnects the session it is handed, + // and that hand-off is no more this test's subject than the borrow's. + Connection c = pool.getConnection(); + int executedByBorrow = rec.executeCalls; + c.close(); + + Assertions.assertNotNull(c, "the borrow must succeed"); + Assertions.assertEquals(executedBefore + 1, executedByBorrow, + "a blank init SQL leaves the configured borrow check standing, reconnect path and all"); + Assertions.assertEquals("SELECT 1", rec.lastExecuted); + } + + @Test + void aReconnectOnBorrowStillRunsTheConnectCheckWhenThatIsWhatIsConfigured() throws Exception { + Recording rec = new Recording(); + // Nothing but the blank init statement and a test on connect: the + // re-established session is still due its check, the built-in one. + BypassedConfiguration props = new BypassedConfiguration(null, " "); + props.setTestOnConnect(true); + props.setInitialSize(1); + props.setMaxAge(1); + ConnectionPool pool = standInPool(props, rec); + Thread.sleep(5); + int checkedBefore = rec.isValidCalls; + + // Read before the return, for the same reason as the test above. + Connection c = pool.getConnection(); + int checkedByBorrow = rec.isValidCalls; + c.close(); + + Assertions.assertNotNull(c, "the borrow must succeed"); + Assertions.assertEquals(checkedBefore + 1, checkedByBorrow, + "a configured test on connect keeps checking the re-established session"); + Assertions.assertEquals(0, rec.executeCalls, "and no emptiness runs alongside it"); + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBorrowSiteTrace.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBorrowSiteTrace.java new file mode 100644 index 0000000000..299175a273 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestBorrowSiteTrace.java @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * The borrow site of a pooled connection is kept as stack frames and only + * formatted into a trace when an abandoned- or suspect-connection report asks + * for one, and the formatted trace is then cached. The fields behind that + * deferred formatting move together on every borrow, and they move without a + * lock wherever {@code con.lock()} is the no-op it is by default - which is + * also the configuration in which the abandoned check runs only when JMX + * invokes it, so the reader formats outside any lock too. Formatting the older + * site and then caching it unconditionally would cache it over a newer borrow + * that completed meanwhile, erase that newer site, and have the next report + * show the previous borrower's call as the current one's. The cache is for + * the site still current and for nobody else. + */ +class TestBorrowSiteTrace { + + private static StackTraceElement[] site(String marker) { + // A deep site keeps the formatting in flight long enough that the + // race below is reached rather than merely possible. + StackTraceElement[] frames = new StackTraceElement[60000]; + for (int i = 0; i < frames.length - 1; i++) { + frames[i] = new StackTraceElement("FillerClass", "fillerMethod" + i, "FillerClass.java", i); + } + frames[frames.length - 1] = new StackTraceElement(marker, "entryMethod", marker + ".java", 1); + return frames; + } + + private static PooledConnection pooledConnection() throws Exception { + PoolProperties properties = new PoolProperties(); + properties.setInitialSize(0); + properties.setTimeBetweenEvictionRunsMillis(-1); + return new PooledConnection(properties, new ConnectionPool(properties)); + } + + @Test + void aDeferredBorrowSiteFormatsIntoTheReport() throws Exception { + PooledConnection con = pooledConnection(); + con.setStackTraceElements(site("FirstBorrowSite")); + + String trace = con.getStackTrace(); + Assertions.assertNotNull(trace); + Assertions.assertTrue(trace.contains("FirstBorrowSite"), + "the report must show the borrow site it was handed"); + Assertions.assertEquals(trace, con.getStackTrace(), + "the formatted trace is cached, so a second reader pays no reformatting"); + } + + @Test + void anOlderSiteBeingFormattedDoesNotEraseANewerOne() throws Exception { + PooledConnection con = pooledConnection(); + StackTraceElement[] stale = site("StaleBorrowSite"); + StackTraceElement[] fresh = site("FreshBorrowSite"); + + // Each round leaves a deep site pending, starts readers formatting it, + // and re-borrows the connection while that formatting runs. However + // the steps interleave, the site of the latest borrow must survive: + // either a formatting finished before the re-borrow (and the re-borrow + // overwrote the cache), or it finished after (and it may not publish + // over the newer site). The latter is the whole point: a reader that + // checks the site is still current and then writes is still open to a + // re-borrow slipping between the two, which is why the publication is + // atomic against the borrow - so here it must hold with two readers + // in flight and no delay to line the steps up. + for (int round = 0; round < 12; round++) { + con.setStackTraceElements(stale); + Thread first = new Thread(con::getStackTrace, "trace-formatter-1"); + Thread second = new Thread(con::getStackTrace, "trace-formatter-2"); + first.start(); + second.start(); + con.setStackTraceElements(fresh); + first.join(10000); + second.join(10000); + + String trace = con.getStackTrace(); + Assertions.assertNotNull(trace, "the fresh borrow site must still be there to report"); + Assertions.assertTrue(trace.contains("FreshBorrowSite"), + "the newest borrow site must never be lost to a formatting of the older one"); + Assertions.assertFalse(trace.contains("StaleBorrowSite"), + "a report of the current borrow must not read as the previous borrower's call"); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java new file mode 100644 index 0000000000..3a32ea3d15 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java @@ -0,0 +1,234 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * A pooled connection reaches its end from several hands: the abandon check + * and a return whose claim on the busy queue lost both call + * {@link PooledConnection#release()}, and a JMX-invoked abandoned check makes + * that overlap reachable with no lock held anywhere - {@code con.lock()} is + * the no-op it is unless the sweeper or useLock is on. The terminal sequence + * release() drives - the chain's {@code disconnected(finalizing=true)}, the + * physical close, the chain teardown at {@link ConnectionPool#finalize} - is + * not a sequence a session can be taken through twice, so the single winner + * must be claimed before any of it runs, not merely settled for the counters + * afterwards. And the claim must precede the teardown, not follow it: the + * flag that tells a {@link PooledConnection#connect()} this connection is + * gone has to read true while the session is still visibly open, or a racing + * reconnect resurrects the very session being closed. + * + * <p>The pool here is a counting subclass that parks the first terminal run + * inside the chain notice - the exact window a second hand used to slip into - + * so each of these properties is checked against a real overlap rather than a + * lucky interleaving.</p> + */ +class TestCloseVsConcurrentRelease { + + /** How long the first terminal run is held inside the chain notice. */ + private static final long HOLD_MS = 1500; + + private static final class Recording { + final AtomicInteger closeCalls = new AtomicInteger(); + } + + /** Counts the terminal sequence's steps and holds the first one partway through. */ + private static final class CountingPool extends ConnectionPool { + final PoolProperties props; + final AtomicInteger disconnectEvents = new AtomicInteger(); + final AtomicInteger finalizations = new AtomicInteger(); + final CountDownLatch insideFinalizing = new CountDownLatch(1); + final CountDownLatch proceed = new CountDownLatch(1); + + CountingPool(PoolProperties properties) throws SQLException { + super(properties); + this.props = properties; + } + + @Override + protected void disconnectEvent(PooledConnection con, boolean finalizing) { + disconnectEvents.incrementAndGet(); + if (finalizing) { + insideFinalizing.countDown(); + try { + proceed.await(HOLD_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + super.disconnectEvent(con, finalizing); + } + + @Override + protected void finalize(PooledConnection con) { + finalizations.incrementAndGet(); + super.finalize(con); + } + } + + private static Connection recordingConnection(Recording rec) { + return (Connection) Proxy.newProxyInstance(TestCloseVsConcurrentRelease.class.getClassLoader(), + new Class<?>[] { Connection.class }, (proxy, method, args) -> { + switch (method.getName()) { + case "close" : + rec.closeCalls.incrementAndGet(); + return null; + case "isClosed" : + return Boolean.FALSE; + case "getAutoCommit" : + return Boolean.TRUE; + case "equals" : + return proxy == args[0]; + case "hashCode" : + return Integer.valueOf(System.identityHashCode(proxy)); + case "toString" : + return "recording-connection"; + default : + return defaultValue(method.getReturnType()); + } + }); + } + + private static Object defaultValue(Class<?> type) { + if (type == boolean.class || type == Boolean.class) { + return Boolean.FALSE; + } + if (type == int.class || type == Integer.class) { + return Integer.valueOf(0); + } + if (type == long.class || type == Long.class) { + return Long.valueOf(0); + } + return null; + } + + /** A connected session whose physical close is counted, on a counting pool. */ + private static PooledConnection connectedTo(CountingPool pool, Recording rec) throws SQLException { + PooledConnection con = new PooledConnection(pool.props, pool); + con.connect(); + return con; + } + + private static CountingPool countingPool(Recording rec) throws SQLException { + PoolProperties props = new PoolProperties(); + props.setJmxEnabled(false); + props.setInitialSize(0); + props.setTimeBetweenEvictionRunsMillis(-1); + props.setValidationInterval(0); + props.setDataSource(Proxy.newProxyInstance(TestCloseVsConcurrentRelease.class.getClassLoader(), + new Class<?>[] { javax.sql.DataSource.class }, (proxy, method, args) -> { + switch (method.getName()) { + case "getConnection" : + return recordingConnection(rec); + case "equals" : + return proxy == args[0]; + case "hashCode" : + return Integer.valueOf(System.identityHashCode(proxy)); + case "toString" : + return "recording-datasource"; + default : + return defaultValue(method.getReturnType()); + } + })); + return new CountingPool(props); + } + + @Test + void aSecondReleaseDuringTheTerminalSequenceActsOnNothing() throws Exception { + Recording rec = new Recording(); + CountingPool pool = countingPool(rec); + PooledConnection con = connectedTo(pool, rec); + + AtomicBoolean firstResult = new AtomicBoolean(); + Thread releaser = new Thread(() -> firstResult.set(con.release()), "first-releaser"); + releaser.start(); + Assertions.assertTrue(pool.insideFinalizing.await(10, TimeUnit.SECONDS), + "the first release must have reached the chain's terminal notice"); + + // The window the abandon check and a losing return fall into: the + // first hand is inside the notice, the session is marked discarded + // but still visibly open. This hand must claim nothing and touch + // nothing. + Assertions.assertFalse(con.release(), + "a release that arrives during the terminal sequence loses the claim"); + Assertions.assertEquals(1, pool.disconnectEvents.get(), + "the terminal notice must have gone through the chain once, not once per hand"); + Assertions.assertEquals(0, rec.closeCalls.get(), "the loser must not close the session"); + Assertions.assertEquals(0, pool.finalizations.get(), "the loser must not tear the chain down"); + + pool.proceed.countDown(); + releaser.join(10000); + Assertions.assertFalse(releaser.isAlive(), "the first release must complete"); + Assertions.assertTrue(firstResult.get(), "the first release is the winner"); + Assertions.assertEquals(1, pool.disconnectEvents.get(), "the terminal notice ran exactly once"); + Assertions.assertEquals(1, pool.finalizations.get(), "the chain teardown ran exactly once"); + Assertions.assertEquals(1, rec.closeCalls.get(), "the session was physically closed exactly once"); + } + + @Test + void aReconnectCannotResurrectASessionBeingTornDown() throws Exception { + Recording rec = new Recording(); + CountingPool pool = countingPool(rec); + PooledConnection con = connectedTo(pool, rec); + + Thread releaser = new Thread(con::release, "first-releaser"); + releaser.start(); + Assertions.assertTrue(pool.insideFinalizing.await(10, TimeUnit.SECONDS), + "the first release must have reached the chain's terminal notice"); + + // While the first hand is still inside the teardown, the flag that + // says this connection is gone must already tell connect() so: settled + // only after the teardown, a connect() reading it now would pass and + // put a new session behind a connection being destroyed - the + // resurrection this ordering is what closes. + Assertions.assertThrows(SQLException.class, con::connect, + "a connection whose release has begun must refuse a reconnect attempt"); + + pool.proceed.countDown(); + releaser.join(10000); + Assertions.assertFalse(releaser.isAlive(), "the first release must complete"); + Assertions.assertEquals(1, pool.disconnectEvents.get(), + "the refused connect must have added nothing to the terminal sequence"); + } + + @Test + void aReleaseAfterTheFirstStaysALossWithoutTouchingAnything() throws Exception { + Recording rec = new Recording(); + CountingPool pool = countingPool(rec); + PooledConnection con = connectedTo(pool, rec); + + // This test is about the sequential second hand, not the held-up + // first one, so the notice does not need to park anyone. + pool.proceed.countDown(); + + Assertions.assertTrue(con.release(), "the first release wins"); + Assertions.assertFalse(con.release(), "every later release loses"); + Assertions.assertEquals(1, pool.disconnectEvents.get(), "the terminal notice ran once, though two hands came"); + Assertions.assertEquals(1, pool.finalizations.get()); + Assertions.assertEquals(1, rec.closeCalls.get(), "the session was closed once, though two hands came"); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverInstantiationLog.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverConnectDiagnostics.java similarity index 64% rename from modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverInstantiationLog.java rename to modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverConnectDiagnostics.java index 43643fbd0a..c8b629ff44 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverInstantiationLog.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDriverConnectDiagnostics.java @@ -17,6 +17,7 @@ package org.apache.tomcat.jdbc.pool; import java.sql.Connection; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -29,20 +30,22 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.apache.juli.logging.LogFactory; - import org.apache.tomcat.jdbc.JdbcPoolBaseTest; /** - * The debug line {@code connectUsingDriver()} writes when it instantiates the - * configured driver prints the JDBC URL of the configuration, and enabling - * DEBUG on this package is a standard support step. A pool whose configuration - * carries its credential inside the URL - the {@code ;PASSWORD=...} pair an H2 - * or PostgreSQL style URL accepts - must therefore not have that credential - * written to the log: the same masking that keeps the configuration dump - * clean ({@code TestPoolProperties}) has to cover this line too, while the - * driver itself still receives the real URL. + * The diagnostics {@code connectUsingDriver()} emits about the configured JDBC + * URL - the debug line it writes when instantiating the driver, and the + * {@link java.sql.SQLException} it throws when a driver declines the URL by + * returning null - print that URL, which legitimately carries an inline + * credential (the {@code ;PASSWORD=...} pair an H2 or PostgreSQL style URL + * accepts, or a {@code ?password=...} query pair). Enabling DEBUG is a standard + * support step and the returned-null failure is logged at the default level on + * every attempt, so a credential in the URL must not reach either surface: the + * same masking that keeps the configuration dump clean + * ({@code TestPoolProperties}) has to cover them, while the driver still + * receives the real URL. */ -class TestDriverInstantiationLog extends JdbcPoolBaseTest { +class TestDriverConnectDiagnostics extends JdbcPoolBaseTest { /** * Captures what the pooled connection class logs. The handler goes in @@ -162,4 +165,72 @@ class TestDriverInstantiationLog extends JdbcPoolBaseTest { "a URL with nothing to mask must print exactly as configured, was: " + line); } } + + /** + * A driver that answers every URL the way the JDBC contract lets a driver + * answer one it does not recognise: {@code connect} returns null. This is + * the persistent-misconfiguration case that makes the returned-null failure + * recur on every attempt, so it is where that failure's message reaches the + * log. + */ + public static class NullReturningDriver implements java.sql.Driver { + @Override + public Connection connect(String url, java.util.Properties info) { + return null; + } + + @Override + public boolean acceptsURL(String url) { + return true; + } + + @Override + public java.sql.DriverPropertyInfo[] getPropertyInfo(String url, java.util.Properties info) { + return new java.sql.DriverPropertyInfo[0]; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public boolean jdbcCompliant() { + return false; + } + + @Override + public Logger getParentLogger() throws java.sql.SQLFeatureNotSupportedException { + throw new java.sql.SQLFeatureNotSupportedException(); + } + } + + @Test + void aDriverThatDeclinesTheUrlIsReportedWithTheUrlMasked() throws Exception { + // The failure is the borrow's own SQLException and the pool logs it at + // the default level, so a driver declining a credential-bearing URL + // must not have that credential written into the message. + String secret = "S3cret-In-The-Declined-Url"; + PoolProperties properties = new PoolProperties(); + properties.setUrl("jdbc:declined:thin:@dbhost:1521:sid?user=appuser&password=" + secret); + properties.setDriverClassName(NullReturningDriver.class.getName()); + properties.setJmxEnabled(false); + properties.setInitialSize(0); + properties.setTimeBetweenEvictionRunsMillis(-1); + PooledConnection con = new PooledConnection(properties, new ConnectionPool(properties)); + + SQLException thrown = Assertions.assertThrows(SQLException.class, con::connect); + String message = thrown.getMessage(); + Assertions.assertTrue(message.contains("password=********"), + "the pool's own failure message must mask the URL, was: " + message); + Assertions.assertTrue(message.contains("jdbc:declined:thin:@dbhost:1521:sid?user=appuser"), + "the URL must stay diagnosable with only its credential masked, was: " + message); + Assertions.assertFalse(message.contains(secret), + "the declined-URL failure must not carry the credential, was: " + message); + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java index 9a02edc2b6..cc8f906b15 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java @@ -151,15 +151,22 @@ class TestShouldForceReconnect { } @Test - void withoutAlternateCredentialsSupportNoBorrowerEverForcesAReconnect() - throws Exception { + void withoutAlternateCredentialsSupportThePoolServesOnlyItsOwnIdentity() throws Exception { properties.setAlternateUsernameAllowed(false); PooledConnection con = pooledConnection(); - loggedInAs(con, ALT_USER, ALT_PASSWD); + loggedInAs(con, DEFAULT_USER, DEFAULT_PASSWD); - Assertions.assertFalse(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD), + Assertions.assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), "with alternates disallowed the credentials of the borrower are ignored, " - + "so the session can never mismatch"); - Assertions.assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + + "and this session already serves the pool's own identity"); + + loggedInAs(con, ALT_USER, ALT_PASSWD); + Assertions.assertTrue(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD), + "but a session carrying another identity than the pool's own is re-established, " + + "however it came to carry it - the flag can be turned at runtime " + + "over a session that was re-established while alternates were allowed"); + Assertions.assertTrue(con.shouldForceReconnect(null, null), + "including for the default borrower, whose claim is exactly the identity " + + "this session lacks"); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java index 9aed87e92c..0484ac8867 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java @@ -341,8 +341,14 @@ class TestJmxMonitor extends JdbcPoolBaseTest { dbProperties.setProperty("injected", "through-the-view"); Assertions.assertNull(ds.getPoolProperties().getDbProperties().getProperty("injected"), "the exposed db-properties must be a view, not the live object"); - // and the pool must still authenticate - the copy is display-side - // only, the connect path keeps its own credentials + // The display path corrupted nothing live: with the fixture password + // back, a borrow succeeds. It would not while the fake secret stood: + // a session authenticated with anything other than the configured + // credentials is re-established for the next borrower rather than + // served, and this secret never existed in the database - the display + // path's silence about a credential cannot be matched by the connect + // path's use of it. + ds.setPassword(""); Assertions.assertDoesNotThrow(() -> ds.getConnection().close()); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
