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 54592653cf279b6c0fc89011391b15dc5bd4a6d1 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 23:20:22 2026 +0100 Answer equals and hashCode on a facade-less pooled connection ProxyConnection's XA dispatch branch matches any method whose declaring class is a supertype of XAConnection, which includes java.lang.Object, so equals() and hashCode() on the connection handle fell into it. With useDisposableConnectionFacade=false (the default facade masks the calls) that meant: NPE on a plain pool (the XA connection is null there), wrong identity on an XA pool (the XAConnection was asked to compare itself against the proxy, so a handle was not equal to itself and two handles over one session shared a hash), and UndeclaredThrowableException via the already-closed SQLException once the connection had been returned. Any HashSet<Connection> of pooled connections broke. The calls are now answered ahead of that branch and ahead of the closed check, with the identity of the connection handle itself, which is exactly the semantics the disposable facade provides when it is enabled: a handle is equal only to itself and its hash is stable, per borrow, even after release. Note this makes identity answerable on a stale handle of a re-armed chain; that ghost-handle reuse is a separate known defect (ProxyConnection report) and is not changed by this commit. The handler-based compare proposed in the defect report would have broken reflexivity in a multi-link chain (the cached handler is not the chain head) and would have resurrected the shared-per-session identity, hence the proxy-argument-based identity here. Regression tests in TestFacadelessProxyIdentity (plain pool, released handle, per-borrow identity over one reused session, HashSet keys, and the XA-pool self-equality case); all fail on the pre-fix code. The TESTING-PLAN note and the TestProxyContract pointer that recorded this behaviour as deliberately unpinned are updated accordingly. --- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 21 ++- .../jdbc/pool/TestFacadelessProxyIdentity.java | 175 +++++++++++++++++++++ .../apache/tomcat/jdbc/pool/TestProxyContract.java | 8 +- 3 files changed, 199 insertions(+), 5 deletions(-) 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 51bb5744e2..232f04904d 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 @@ -23,11 +23,15 @@ import java.sql.SQLException; import javax.sql.XAConnection; /** * A ProxyConnection object is the bottom most interceptor that wraps an object of type - * {@link PooledConnection}. The ProxyConnection intercepts three methods: + * {@link PooledConnection}. The ProxyConnection intercepts, among the calls it forwards, + * the following methods: * <ul> * <li>{@link java.sql.Connection#close()} - returns the connection to the pool. May be called multiple times.</li> * <li>{@link java.lang.Object#toString()} - returns a custom string for this object</li> * <li>{@link javax.sql.PooledConnection#getConnection()} - returns the underlying connection</li> + * <li>{@link java.lang.Object#equals(Object)} and {@link java.lang.Object#hashCode()} - + * answered with the identity of the connection handle, so that the {@code Object} + * contract holds even when no facade sits in front of this interceptor</li> * </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. @@ -128,6 +132,21 @@ public class ProxyConnection extends JdbcInterceptor { if (compare(ISCLOSED_VAL,method)) { return Boolean.valueOf(isClosed()); } + if (compare(EQUALS_VAL,method)) { + // With the DisposableConnectionFacade switched off, Object methods + // reach this interceptor, and the XA branch below would dispatch + // them at a null XA connection (or at the XA connection of another + // borrower) instead of answering them. Object.equals must answer + // false rather than throw for a null or foreign argument, and it + // must stay answerable on a released handle, so the identity of + // the handle itself is what compares - mirroring the semantics of + // the facade when it is enabled. + Object other = (args == null || args.length == 0) ? null : args[0]; + return Boolean.valueOf(proxy == other); + } + if (compare(HASHCODE_VAL,method)) { + return Integer.valueOf(System.identityHashCode(proxy)); + } if (compare(CLOSE_VAL,method)) { if (connection==null) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadelessProxyIdentity.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadelessProxyIdentity.java new file mode 100644 index 0000000000..fbe30d3067 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadelessProxyIdentity.java @@ -0,0 +1,175 @@ +/* + * 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.sql.Connection; +import java.util.HashSet; +import java.util.Set; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The pool's own {@code equals}/{@code hashCode} handling on the handle it + * hands out, below the {@link DisposableConnectionFacade}. With the facade + * enabled (the default) the facade answers these calls and + * {@link ProxyConnection} never sees them; with the facade switched off the + * calls reach {@code ProxyConnection}, which used to forward them down the + * XA dispatch branch - on a plain pool that resolves the XA connection to + * {@code null} and the call threw, and on an XA pool it compared against + * the underlying {@code XAConnection} instead of the handle. Any + * {@code HashSet<Connection>} of pooled connections broke, and even after + * {@code close()} the contract still has to hold, so the calls must stay + * answerable on a released handle too. + * + * <p>Identity semantics are pinned per borrow, matching the facade-on + * behaviour asserted by {@link TestProxyContract}: a handle is equal only + * to itself, even when a later borrow maps to the same physical session.</p> + */ +class TestFacadelessProxyIdentity extends JdbcPoolBaseTest { + + private DataSource newDataSourceWithoutFacade() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setUseDisposableConnectionFacade(false); + return ds; + } + + @Test + void aLiveHandleHonoursEqualsHashCodeContract() throws Exception { + DataSource ds = newDataSourceWithoutFacade(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + // Pre-fix these threw because the Object methods fell into the XA + // dispatch branch with a null XA connection. + Assertions.assertTrue(pooled.equals(pooled), "a handle must be equal to itself"); + Assertions.assertFalse(pooled.equals(null), + "Object.equals(null) must answer false, not throw"); + Assertions.assertFalse(pooled.equals("a foreign object"), + "equals against a non-proxy must answer false, not throw"); + Object foreignProxy = java.lang.reflect.Proxy.newProxyInstance( + getClass().getClassLoader(), + new Class<?>[] { Runnable.class }, + (p, m, a) -> null); + Assertions.assertFalse(pooled.equals(foreignProxy), + "equals against a foreign dynamic proxy must answer false"); + int hash = pooled.hashCode(); + Assertions.assertEquals(hash, pooled.hashCode(), "hashCode must be stable"); + pooled.close(); + } + + @Test + void aReleasedHandleKeepsTheContract() throws Exception { + DataSource ds = newDataSourceWithoutFacade(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + Assertions.assertTrue(pooled.equals(pooled)); + int hash = pooled.hashCode(); + pooled.close(); + + // hashCode/equals are identity, not liveness - a released handle + // still has to answer them (pre-fix: UndeclaredThrowableException + // caused by the "Connection has already been closed" SQLException). + Assertions.assertEquals(hash, pooled.hashCode(), + "closing must not change the hash code of a handle"); + Assertions.assertTrue(pooled.equals(pooled), + "closing must not change the identity of a handle"); + Assertions.assertFalse(pooled.equals(null), + "a released handle must keep the equals(null) contract"); + } + + @Test + void twoBorrowsOverOneSessionAreDistinctHandles() throws Exception { + DataSource ds = newDataSourceWithoutFacade(); + ds.createPool(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 42); + Assertions.assertTrue(first.equals(first)); + int firstHash = first.hashCode(); + first.close(); + + try (Connection second = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(42), sessionMarker(second), + "the test must actually be looking at one reused physical session"); + Assertions.assertFalse(first.equals(second), + "two borrows are two handles even over one physical session"); + Assertions.assertFalse(second.equals(first)); + Assertions.assertNotEquals(firstHash, second.hashCode(), + "two handles must not share an identity hash"); + } + } + + @Test + void pooledConnectionsUsableAsHashCollectionKeys() throws Exception { + DataSource ds = newDataSourceWithoutFacade(); + ds.createPool(); + + Set<Connection> seen = new HashSet<>(); + Connection a = ds.getConnection(); + Connection b = ds.getConnection(); + Assertions.assertTrue(seen.add(a), "first insert must succeed"); + Assertions.assertTrue(seen.add(b), "second insert must succeed"); + Assertions.assertFalse(seen.add(a), "a handle already in the set must not re-insert"); + Assertions.assertTrue(seen.contains(a), "collection lookup must find the handle"); + Assertions.assertTrue(seen.contains(b)); + a.close(); + b.close(); + Assertions.assertTrue(seen.contains(a), + "a released handle must still hash to its bucket"); + } + + @Test + void anXaCapableHandleKeepsIdentityForItself() throws Exception { + 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); + ds.getPoolProperties().setUseDisposableConnectionFacade(false); + manage(ds); + ds.createPool(); + + Connection first = ds.getConnection(); + // Pre-fix the calls were dispatched at the live XAConnection: + // a handle was not equal to itself (the XA connection was asked + // to compare itself against the proxy) and two handles over one + // session shared the XA connection's hash. + Assertions.assertTrue(first.equals(first), + "an XA-capable handle must be equal to itself"); + Assertions.assertFalse(first.equals(null)); + int firstHash = first.hashCode(); + Assertions.assertEquals(firstHash, first.hashCode(), "hashCode must be stable"); + first.close(); + + try (Connection second = ds.getConnection()) { + Assertions.assertTrue(second.equals(second)); + Assertions.assertFalse(first.equals(second), + "two borrows are two handles even over one physical session"); + Assertions.assertNotEquals(firstHash, second.hashCode(), + "two handles must not share the XA connection's hash"); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestProxyContract.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestProxyContract.java index 8e5c0b3857..7eb64c67ec 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestProxyContract.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestProxyContract.java @@ -34,10 +34,10 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; * for the callers that need it. * * <p>Scenario harvested from the retired legacy classes - * {@code MultipleCloseTest} and {@code EqualsHashCodeTest}. Note that - * the pool's own {@code equals}/{@code hashCode} handling <em>below</em> - * the facade (with {@code useDisposableConnectionFacade=false}) is not - * covered here on purpose - see TESTING-PLAN.md.</p> + * {@code MultipleCloseTest} and {@code EqualsHashCodeTest}. The pool's own + * {@code equals}/{@code hashCode} handling <em>below</em> the facade (with + * {@code useDisposableConnectionFacade=false}) is pinned separately in + * {@link TestFacadelessProxyIdentity}.</p> */ class TestProxyContract extends JdbcPoolBaseTest { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
