This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 67532e3a59f5591551630c53077865844f4c3fec Author: opencode <[email protected]> AuthorDate: Fri Oct 30 18:52:51 2026 +0100 Answer isValid with false on a discarded handle isValid() is the probe for liveness, and the JDBC contract answers a closed connection with false rather than with an exception. A handle whose session the pool discarded while it was still borrowed violated that: the probe traveled the interceptor chain and hit ProxyConnection's closed guard, so an application, health check or pool doing the spec-conformant thing got "Connection has already been closed." precisely on the dead connection it was trying to detect. Three paths reach that state without the application ever calling close(), and none of them seals the disposable facade, which is where the pool already answers this probe with false for a returned handle: the abandoned connection check reclaiming a session from a borrower that never returned it, a forced pool close tearing sessions away from their borrowers, and an interceptor calling setDiscarded(true) to have the session closed on return. The two closed states therefore disagreed with each other, and with the driver, which returns false for its own closed connection. ProxyConnection now answers the probe with false when the handle is closed, beside the branch that answers isClosed() and ahead of the guard that threw. It sits at the terminus of the chain because that is the link holding the closed and discarded state; the facade cannot answer for it, since it knows only whether its own link was sealed and not whether the session underneath was discarded. A handle that is not closed falls through as before, so the probe still really probes the session: a driver-side failure surfaces rather than becoming a quiet false, and a driver that rejects a negative timeout, as the contract requires although not every driver enforces it, still says so. The interception performs no argument validation of its own, because the sealed facade has never consulted the argument either and the two closed states are meant to agree. Add TestIsValidOnDiscardedHandle, driving the abandoned connection check, a forced pool close, and a configured interceptor that marks the session discarded, asserting false from the probe in each while the surrounding answers are untouched; a sealed handle keeps answering false as it always did; and a strict driver fixture pins that a live handle passes the probe through and that a closed handle never reaches the driver. The interceptor marking is put in JdbcPoolBaseTest beside the existing one, so the state is produced through a configured chain rather than by setting the flag directly. No changelog entry. --- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 11 + .../org/apache/tomcat/jdbc/JdbcPoolBaseTest.java | 35 +++ .../jdbc/pool/TestIsValidOnDiscardedHandle.java | 283 +++++++++++++++++++++ 3 files changed, 329 insertions(+) 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 c58ad67c6d..b3ec881f83 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 @@ -199,6 +199,17 @@ public class ProxyConnection extends JdbcInterceptor { if (compare(ISCLOSED_VAL, method)) { return Boolean.valueOf(isClosed()); } + if (compare(ISVALID_VAL, method) && isClosed()) { + // isValid() is the probe for liveness, and the JDBC contract + // answers a closed connection with false rather than with an + // exception. A session the pool discarded while its borrower + // still held the handle - an abandon, a forced pool close, an + // interceptor marking it discarded - does not seal the + // disposable facade, so without this branch the probe would + // reach the closed guard below and throw. The guard answers + // false, as the sealed facade does; a live handle falls through. + return Boolean.FALSE; + } if (compare(EQUALS_VAL, method)) { // With the DisposableConnectionFacade switched off, Object methods // reach this interceptor, and the XA branch below would dispatch diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/JdbcPoolBaseTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/JdbcPoolBaseTest.java index d3c49df9a4..5da1bc90cf 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/JdbcPoolBaseTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/JdbcPoolBaseTest.java @@ -17,6 +17,7 @@ package org.apache.tomcat.jdbc; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.DriverManager; @@ -184,6 +185,40 @@ public abstract class JdbcPoolBaseTest { } } + /* + * An interceptor that drives the documented "close this session when it + * is returned" hook through the configured chain rather than by touching + * the pooled connection directly: the first isValid() call that passes + * through it is delegated as usual - answered by the driver, while the + * session is still live - and as it returns the interceptor marks the + * connection discarded, leaving the borrower holding a handle whose + * session is out of service. It is configured through the + * jdbcInterceptors class-name list, so it must stay a public static + * class with a no-arg constructor. + */ + public static class DiscardMarkingInterceptor extends JdbcInterceptor { + private PooledConnection held; + private volatile boolean marked; + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + held = con; + marked = false; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) + throws Throwable { + Object result = super.invoke(proxy, method, args); + if (!marked && "isValid".equals(method.getName()) + && held != null) { + marked = true; + held.setDiscarded(true); + } + return result; + } + } + /* * The driver's own object behind a pooled connection's statement. For * a prepared statement this is the honest oracle for "the session was 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 new file mode 100644 index 0000000000..e5290c58a3 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestIsValidOnDiscardedHandle.java @@ -0,0 +1,283 @@ +/* + * 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.io.PrintWriter; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The JDBC liveness probe, {@code isValid(int)}, answers a closed + * connection with {@code false}: it is the probe <em>for</em> liveness and + * must not throw for a connection that is merely closed. The pool has two + * closed states for a handle. The application returned it, which seals the + * disposable facade; or the pool discarded the session while the handle was + * still borrowed - through the abandoned check, through a forced pool + * close, or through an interceptor's {@code setDiscarded(true)} - which + * does not seal it. Both states must answer the probe with {@code false}, + * and only the sealed one was covered. A handle that is still live must + * 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. + */ +class TestIsValidOnDiscardedHandle extends JdbcPoolBaseTest { + + @Test + void anAbandonedBorrowAnswersTheValidityProbeWithFalse() throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration properties = ds.getPoolProperties(); + properties.setRemoveAbandoned(true); + properties.setRemoveAbandonedTimeout(1); + properties.setInitialSize(0); + ds.createPool(); + + Connection abandoned = ds.getConnection(); + PooledConnection pooled = + interceptorOf(abandoned, ProxyConnection.class).getDelegateConnection(); + // Backdate the borrow past the abandon timeout and run the check the + // pool's cleaner thread runs. + pooled.setTimestamp(0L); + ds.getPool().checkAbandoned(); + + Assertions.assertTrue(pooled.isDiscarded(), + "precondition: the pool must have discarded the borrowed session"); + awaitSessions(0); // and taken the physical session out of service + + Assertions.assertFalse(abandoned.isValid(1), + "a session the pool discarded while it was still borrowed must answer" + + " the validity probe with false instead of throwing"); + Assertions.assertFalse(abandoned.isValid(0), + "the answer is the same whatever the timeout argument"); + Assertions.assertFalse(abandoned.isValid(-1), + "and it does not look at a negative timeout either, exactly like the" + + " sealed facade's answer"); + Assertions.assertTrue(abandoned.isClosed(), + "the handle keeps reporting itself closed"); + abandoned.close(); // the late return stays the documented no-op + } + + @Test + void anInterceptorDiscardedSessionAnswersTheValidityProbeWithFalse() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + DiscardMarkingInterceptor.class.getName()); + ds.createPool(); + + Connection held = ds.getConnection(); + // The configured interceptor marks the session to be closed when it + // is returned, while the borrower still holds the handle; it does so + // as the first probe passes through it, having first delegated that + // probe to the driver, which is what the live session answers. + Assertions.assertTrue(held.isValid(1), + "precondition: the first probe finds the session live, at the driver"); + Assertions.assertTrue(held.isClosed(), + "precondition: the interceptor's marking must have reached the handle"); + + Assertions.assertFalse(held.isValid(1), + "a session marked for discard must answer the validity probe with false" + + " to the borrower that still holds it"); + Assertions.assertEquals(1, poolSessions(), + "precondition: the physical session is still open until it is returned"); + + held.close(); + awaitSessions(0); // and out of service once it has been returned + } + + @Test + void aHandleTornAwayByAForcedPoolCloseAnswersTheValidityProbeWithFalse() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection held = ds.getConnection(); + // A forced shutdown tears busy sessions away from their borrowers. + ds.close(true); + awaitSessions(0); + + Assertions.assertFalse(held.isValid(1), + "a handle whose session the shutdown discarded must answer the validity" + + " probe with false instead of throwing"); + Assertions.assertTrue(held.isClosed()); + held.close(); // and its close stays the documented no-op + } + + @Test + void aSealedHandleKeepsAnsweringTheValidityProbeWithFalse() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection returned = ds.getConnection(); + returned.close(); + + Assertions.assertFalse(returned.isValid(1), + "the sealed facade's false answer must be untouched by the closed" + + " guard added in the chain below it"); + Assertions.assertFalse(returned.isValid(-1), + "the sealed answer does not look at the timeout argument"); + Assertions.assertTrue(returned.isClosed()); + } + + @Test + void aLiveHandleAnswersTheValidityProbeThroughItsDriver() throws Exception { + StrictValidityDataSource strict = new StrictValidityDataSource(getUrl()); + DataSource ds = newDataSource(); + ds.getPoolProperties().setDataSource(strict); + ds.createPool(); + + Connection pooled = ds.getConnection(); + try { + Assertions.assertTrue(pooled.isValid(1), + "the driver's positive answer must reach the caller"); + + // The closed guard added for discarded handles is a guard, not a + // timeout validator: on a live handle the probe travels to the + // driver, and a driver that honors the contract's throw for a + // negative timeout must keep throwing it through the pool. + Assertions.assertThrows(SQLException.class, () -> pooled.isValid(-1), + "the driver's refusal of a negative timeout must not be swallowed"); + + strict.refuseValidity(); + Assertions.assertThrows(SQLException.class, () -> pooled.isValid(1), + "a driver that cannot vouch for its session must say so; the pool" + + " must not replace the failure with a quiet false"); + strict.acceptValidity(); + Assertions.assertTrue(pooled.isValid(1), + "and its recovery reaches the caller as well"); + + // Once the handle is closed, the pool answers the probe itself. + strict.refuseValidity(); + interceptorOf(pooled, ProxyConnection.class).getDelegateConnection().setDiscarded(true); + int askedBefore = strict.validityProbes(); + Assertions.assertFalse(pooled.isValid(-1), + "the closed answer is the pool's own"); + Assertions.assertEquals(askedBefore, strict.validityProbes(), + "and a closed handle answers the probe without consulting the driver"); + strict.acceptValidity(); + } finally { + pooled.close(); + } + } + + /* + * A backing data source whose sessions honor the JDBC contract more + * strictly than H2 does - H2 answers isValid(-1) with true rather than + * throwing what the specification prescribes. These sessions throw the + * prescribed SQLException for a negative timeout, can be made to fail + * the probe outright, and count every probe so a test can see whether + * the driver was asked at all. + */ + private static final class StrictValidityDataSource implements javax.sql.DataSource { + + private final String url; + private final AtomicInteger probes = new AtomicInteger(); + private volatile boolean refuseValidity; + + StrictValidityDataSource(String url) { + this.url = url; + } + + void refuseValidity() { + refuseValidity = true; + } + + void acceptValidity() { + refuseValidity = false; + } + + int validityProbes() { + return probes.get(); + } + + @Override + public Connection getConnection() throws SQLException { + return strictSession(DriverManager.getConnection(url)); + } + + @Override + public Connection getConnection(String username, String password) throws SQLException { + return strictSession(DriverManager.getConnection(url, username, password)); + } + + private Connection strictSession(Connection real) { + return (Connection) Proxy.newProxyInstance( + StrictValidityDataSource.class.getClassLoader(), + new Class<?>[] { Connection.class }, + (proxy, method, args) -> { + if ("isValid".equals(method.getName())) { + probes.incrementAndGet(); + if (((Number) args[0]).intValue() < 0) { + throw new SQLException("this driver refuses a negative timeout"); + } + if (refuseValidity) { + throw new SQLException("this driver cannot vouch for the session"); + } + } + try { + return method.invoke(real, args); + } catch (InvocationTargetException x) { + throw x.getTargetException() != null ? x.getTargetException() : x; + } + }); + } + + @Override + public PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(PrintWriter out) { + // NOOP + } + + @Override + public void setLoginTimeout(int seconds) { + // NOOP + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + + @Override + public <T> T unwrap(Class<T> iface) throws SQLException { + throw new SQLFeatureNotSupportedException(); + } + + @Override + public boolean isWrapperFor(Class<?> iface) throws SQLException { + return false; + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
