This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0a23e1cb231024f6e6b7f51fb0e6709439bb545a Author: opencode <[email protected]> AuthorDate: Fri Oct 30 19:12:28 2026 +0100 Document that the handle's pooled connection listeners go nowhere The handle the pool hands out is a JDK proxy implementing javax.sql.PooledConnection, and four of that interface's methods - addConnectionEventListener, removeConnectionEventListener, addStatementEventListener and removeStatementEventListener - can never do anything on it, yet the class javadoc advertised the interface without ever saying so. A caller written against the specification, which declares no checked exception on those methods, therefore learned about the gap only from an UndeclaredThrowableException carrying the pool's refusal. The refusal itself is not new and stays. It came in with 22c1b56836, which replaced a raw null pointer with a diagnosable SQLException naming the method, and the measurements behind this change confirm that was the right outcome and that the options beyond it are worse. Registering and then never notifying is the specification's promise, so documented no-ops would be the larger contract violation: a container that registers for connectionErrorOccurred to learn its session was abandoned would be told nothing. A real listener list has nowhere honest to fire either. close() on this handle recycles a session that stays open and owned by the pool, and the next borrower is handed a new handle, so the event that says the logical connection has been closed would report a physical close that did not happen; the pool by default observes no statements at all, so statementClosed has no point of delivery; and the listener the specification has in mind is the connection pool manager, which for this pool is the pool itself, already making the deactivate-and-reuse decisions the event exists to prompt. What remains possible - notifying connectionErrorOccurred when the pool discards a session out from under its borrower, or routing registration to the driver's pooled connection on a pool backed by a ConnectionPoolDataSource - is an enhancement with its own semantics questions, not a defect to close here. The class javadoc now states the interface the handle advertises, what each of the four methods does on an XA-backed pool, on a plain pool and on a pool backed by a ConnectionPoolDataSource, what it does once the session has been discarded or the handle closed, how the failure reaches a caller of the interface, and why the refusal is deliberate rather than unfinished work. Add TestPooledConnectionListenerContract, pinning that contract through the interface as a portable caller would call it: the refusal names the missing configuration and the method, refusals leave the handle fully usable and it returns cleanly, an XA-backed pool sends the connection listener pair to the driver's XA connection and surfaces the driver's own answer for the statement pair rather than the pool's refusal, and a ConnectionPoolDataSource-backed pool really does hold the driver's pooled connection while still refusing the registration. No changelog entry: no behavior changed. --- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 22 ++ .../pool/TestPooledConnectionListenerContract.java | 325 +++++++++++++++++++++ 2 files changed, 347 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 b3ec881f83..733141fa95 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 @@ -36,6 +36,28 @@ import javax.sql.XAConnection; * </ul> * By default method comparisons is done on a String reference level, unless the * {@link PoolConfiguration#setUseEquals(boolean)} has been called with a <code>true</code> argument. + * <p> + * The connection handle the pool hands out implements {@link javax.sql.PooledConnection} (and + * {@link javax.sql.XAConnection} when the pool is backed by an {@link javax.sql.XADataSource}). The pool keeps no + * connection or statement event listeners of its own, so the four listener methods of + * {@code javax.sql.PooledConnection} - {@code addConnectionEventListener}, {@code removeConnectionEventListener}, + * {@code addStatementEventListener} and {@code removeStatementEventListener} - are answered by the XA-family + * dispatch in {@link #invoke(Object, Method, Object[])}: where the handle is itself an {@code XAConnection} they + * reach the XA connection the driver created, whose implementation decides what registering them means; where it + * is not, the call has no dispatch target and each of the four fails with an {@link SQLException} naming the + * method. The refusal also stands on a pool backed by a {@link javax.sql.ConnectionPoolDataSource}: the pool does + * hold the driver's own pooled connection there, but it holds it for its own session management and does not + * route these calls to it. A handle whose session the pool discarded while it was still borrowed, or which its + * borrower has already closed, answers instead with the closed-connection failure the handle gives in those + * states. Either failure reaches a caller of the interface - the interface declares no checked exception - as an + * {@link java.lang.reflect.UndeclaredThrowableException} carrying it. That refusal is deliberate rather than + * unfinished work. The specification has the connection pool manager register through these methods, to be told + * to deactivate the logical handle and reuse or retire the physical session - decisions this pool makes and + * carries out itself: {@code close()} on the handle recycles a session that stays open and owned by the pool, and + * the next borrower is handed a new handle, so this handle is not the long-lived producer object whose listener + * list the specification describes; and where the pool does not observe statements - as it does not by default - + * a {@code statementClosed} event has no point from which it could be delivered. Accepting a registration it + * could not notify faithfully would only hide from the caller that no notification will ever arrive. */ public class ProxyConnection extends JdbcInterceptor { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionListenerContract.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionListenerContract.java new file mode 100644 index 0000000000..497f85540c --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionListenerContract.java @@ -0,0 +1,325 @@ +/* + * 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.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.UndeclaredThrowableException; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.logging.Logger; + +import javax.sql.ConnectionEvent; +import javax.sql.ConnectionEventListener; +import javax.sql.ConnectionPoolDataSource; +import javax.sql.PooledConnection; +import javax.sql.StatementEvent; +import javax.sql.StatementEventListener; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * Pins the contract the {@link ProxyConnection} class documentation states + * for the four listener methods of {@link javax.sql.PooledConnection}, which + * every handle the pool hands out implements. The pool keeps no connection or + * statement event listeners of its own, and the handle is not the long-lived + * producer object the specification's listener list belongs to - the pool is + * itself the connection pool manager those events exist to notify, close + * recycles a session that stays open, and getConnection() hands out the raw + * physical session - so on every pool whose handle is not an + * {@code XAConnection} each of the four methods must fail as the documented + * refusal: an {@link SQLException} naming the missing XA configuration and the + * method, wrapped by the proxy in an {@link UndeclaredThrowableException} + * because the interface declares no checked exception, and leaving the handle + * fully usable. That includes a pool backed by a + * {@code ConnectionPoolDataSource}: the pool holds the driver's own pooled + * connection in that configuration, but refuses these calls rather than route + * them to it. On an XA pool the same calls must not be refused by the pool: + * they reach the XA connection the driver created, and whatever that + * implementation answers - accepting the registration or refusing in its own + * words - is the driver's decision, never the pool's refusal. + * + * <p>The diagnosability of the refusal itself (SQLException rather than a raw + * NullPointerException, commit 22c1b56836) is pinned in + * {@code TestXaBranchDispatch}; this class pins the contract the documentation + * promises, so the documented statement cannot silently drift.</p> + */ +class TestPooledConnectionListenerContract extends JdbcPoolBaseTest { + + private static final ConnectionEventListener NOOP_CONNECTION_LISTENER = + new ConnectionEventListener() { + @Override + public void connectionClosed(ConnectionEvent e) { + /* the contract under test never notifies a listener */ + } + @Override + public void connectionErrorOccurred(ConnectionEvent e) { + /* the contract under test never notifies a listener */ + } + }; + + private static final StatementEventListener NOOP_STATEMENT_LISTENER = + new StatementEventListener() { + @Override + public void statementClosed(StatementEvent e) { + /* the contract under test never notifies a listener */ + } + @Override + public void statementErrorOccurred(StatementEvent e) { + /* the contract under test never notifies a listener */ + } + }; + + /* The four listener methods, called the way a portable caller calls them. */ + private static Runnable[] listenerCalls(PooledConnection asPooled) { + return new Runnable[] { + () -> asPooled.addConnectionEventListener(NOOP_CONNECTION_LISTENER), + () -> asPooled.removeConnectionEventListener(NOOP_CONNECTION_LISTENER), + () -> asPooled.addStatementEventListener(NOOP_STATEMENT_LISTENER), + () -> asPooled.removeStatementEventListener(NOOP_STATEMENT_LISTENER) }; + } + + private static String[] listenerMethodNames() { + return new String[] { + "addConnectionEventListener", "removeConnectionEventListener", + "addStatementEventListener", "removeStatementEventListener" }; + } + + private XADataSource newXaDataSource() { + org.h2.jdbcx.JdbcDataSource inner = new org.h2.jdbcx.JdbcDataSource(); + inner.setUrl(getUrl()); + inner.setUser("sa"); + inner.setPassword(""); + XADataSource ds = new XADataSource(); + ds.setDriverClassName("org.h2.Driver"); + ds.getPoolProperties().setJmxEnabled(false); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setDataSource(inner); + manage(ds); + return ds; + } + + /* + * A ConnectionPoolDataSource-only face of H2's data source. The pool + * chooses the back end by an instanceof ladder that tries XA first and + * H2's own data source implements all three faces, so hiding the other + * two is what makes the pooled-connection branch reachable in tests. + */ + private static final class PooledConnectionSourceOnly implements ConnectionPoolDataSource { + private final org.h2.jdbcx.JdbcDataSource inner; + + PooledConnectionSourceOnly(org.h2.jdbcx.JdbcDataSource inner) { + this.inner = inner; + } + + @Override + public PooledConnection getPooledConnection() throws SQLException { + // H2 hands out its XA connection for either face of itself + return inner.getXAConnection(); + } + + @Override + public PooledConnection getPooledConnection(String user, String password) throws SQLException { + return inner.getXAConnection(user, password); + } + + @Override + public PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(PrintWriter out) { + // the wrapper configures nothing + } + + @Override + public void setLoginTimeout(int seconds) { + // the wrapper configures nothing + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() { + return null; + } + } + + /* Unwraps whatever a direct interface call surfaced down to its root. */ + private static Throwable rootCause(Throwable thrown) { + Throwable root = thrown; + while ((root instanceof UndeclaredThrowableException + || root instanceof InvocationTargetException) + && root.getCause() != null) { + root = root.getCause(); + } + return root; + } + + @Test + void thePlainPoolHandleRefusesAllFourListenerMethodsNamingTheMissingXaConnection() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + // The premise the class javadoc is about: the handle really does + // advertise the interface whose methods these are. + Assertions.assertInstanceOf(PooledConnection.class, pooled, + "the plain-pool handle must implement javax.sql.PooledConnection"); + Assertions.assertFalse(pooled instanceof javax.sql.XAConnection, + "a plain pool must not hand out a handle that is an XA connection"); + + PooledConnection asPooled = (PooledConnection) pooled; + Runnable[] calls = listenerCalls(asPooled); + String[] names = listenerMethodNames(); + for (int i = 0; i < calls.length; i++) { + UndeclaredThrowableException thrown = Assertions.assertThrows( + UndeclaredThrowableException.class, calls[i]::run, + names[i] + " on a plain pool must surface as the proxy's wrap of" + + " the checked refusal, the interface declaring no checked exception"); + Throwable root = rootCause(thrown); + Assertions.assertInstanceOf(SQLException.class, root, + names[i] + " must fail as an SQLException, not " + + root.getClass().getName()); + Assertions.assertTrue(root.getMessage().contains("not an XA connection"), + names[i] + " must name the missing XA configuration: " + root.getMessage()); + Assertions.assertTrue(root.getMessage().contains( + "method [" + names[i] + "] cannot be dispatched"), + names[i] + " must name the method it cannot dispatch: " + + root.getMessage()); + } + } + } + + @Test + void refusedListenerRegistrationsLeaveThePlainPoolHandleFullyUsable() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + PooledConnection asPooled = (PooledConnection) pooled; + for (Runnable call : listenerCalls(asPooled)) { + Assertions.assertThrows(UndeclaredThrowableException.class, call::run, + "the refusal must still be the documented refusal"); + } + + // A refused registration is a refused call, not a damaged handle: + // the refusal throws from inside invoke() with nothing on the pool + // side changed, so the borrower's unit of work must continue. + Assertions.assertFalse(pooled.isClosed()); + try (Statement st = pooled.createStatement()) { + Assertions.assertTrue(st.execute("SELECT 1")); + } + pooled.close(); + Assertions.assertEquals(1, ds.getIdle(), + "the refusals must not have disturbed the return of the session"); + Assertions.assertEquals(1, poolSessions()); + } + + @Test + void anXaPoolDispatchesTheListenerMethodsToTheDriverXaConnectionInsteadOfRefusing() + throws Exception { + XADataSource ds = newXaDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + // The premise for the other half of the documented contract. + Assertions.assertInstanceOf(javax.sql.XAConnection.class, pooled, + "an XA pool must hand out a handle that is an XA connection"); + + PooledConnection asPooled = (PooledConnection) pooled; + // The connection-event pair is accepted by H2's XA connection: a + // normal return proves the call reached the driver implementation + // and was not refused by the pool. + asPooled.addConnectionEventListener(NOOP_CONNECTION_LISTENER); + asPooled.removeConnectionEventListener(NOOP_CONNECTION_LISTENER); + + // H2's XA connection answers the statement-event pair with an + // UnsupportedOperationException of its own. What the contract + // requires is that the answer is the driver's, not the pool's + // refusal; the driver refusing in its own words is the proof the + // call was dispatched to the XA connection. + Runnable[] calls = listenerCalls(asPooled); + String[] names = listenerMethodNames(); + for (int i = 2; i < calls.length; i++) { + Throwable thrown = Assertions.assertThrows(Throwable.class, calls[i]::run, + names[i] + " must reach the driver's implementation, which answers" + + " it, instead of returning against a driver that refuses it"); + Throwable root = rootCause(thrown); + Assertions.assertNotEquals(SQLException.class, root.getClass(), + names[i] + " must not carry the pool's refusal on an XA pool: " + root); + Assertions.assertInstanceOf(UnsupportedOperationException.class, root, + names[i] + " must surface the driver implementation's own answer," + + " was " + root.getClass().getName() + ": " + root.getMessage()); + } + } + } + + @Test + void aConnectionPoolDataSourceBackedPoolHoldsTheDriversPooledConnectionAndStillRefusesTheListenerMethods() + throws Exception { + org.h2.jdbcx.JdbcDataSource inner = new org.h2.jdbcx.JdbcDataSource(); + inner.setUrl(getUrl()); + inner.setUser("sa"); + inner.setPassword(""); + DataSource ds = newDataSource(); + // The configured data source outranks the URL: the pool takes the + // pooled-connection branch of its configuration ladder. + ds.getPoolProperties().setDataSource(new PooledConnectionSourceOnly(inner)); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + Assertions.assertFalse(pooled instanceof javax.sql.XAConnection, + "a pooled-connection data source must not make the handle an XA connection"); + + // The premise of the documented outcome: unlike the plain pool, + // this configuration really does hold the driver's own pooled + // connection - the class documentation may not claim otherwise. + ProxyConnection handler = interceptorOf(pooled, ProxyConnection.class); + org.apache.tomcat.jdbc.pool.PooledConnection internal = + handler.getDelegateConnection(); + Field cpdsField = org.apache.tomcat.jdbc.pool.PooledConnection.class + .getDeclaredField("cpdsConnection"); + cpdsField.setAccessible(true); + Assertions.assertInstanceOf(javax.sql.PooledConnection.class, cpdsField.get(internal), + "the pool must hold the driver's pooled connection in this configuration"); + + // The documented outcome: the refusal stands as on the plain pool; + // the calls are not routed to the held driver object. + PooledConnection asPooled = (PooledConnection) pooled; + UndeclaredThrowableException thrown = Assertions.assertThrows( + UndeclaredThrowableException.class, + () -> asPooled.addConnectionEventListener(NOOP_CONNECTION_LISTENER), + "addConnectionEventListener must fail as it does on the plain pool"); + Throwable root = rootCause(thrown); + Assertions.assertInstanceOf(SQLException.class, root, + "the refusal must remain an SQLException, was " + root.getClass().getName()); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
