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 c258dc9d5e5494f7eb09897b373f75a487567579 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 09:50:45 2026 +0100 Always use the disposable connection facade; deprecate useDisposableConnectionFacade With the facade switched off, the pool handed out a proxy onto the pooled connection's cached interceptor chain, which is re-armed for the next borrower. A handle kept after close() therefore stayed usable: its close() returned - or its calls drove - the connection the current borrower was still using, including on the XA proxy class. The facade's documented purpose is safety, it was never documented as a performance switch, and measuring shows switching it off saves only one small object allocation per borrow. The pool now always installs the facade. The property is deprecated, has no effect, but is still accepted and stored so existing configurations keep loading. Tests: the facadeless identity suite is replaced by TestFacadeAlwaysUsed (plain and XA stale-handle pins, released-handle contract) and a factory round-trip pin for the deprecated attribute. --- modules/jdbc-pool/doc/jdbc-pool.xml | 12 +- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 18 +-- .../apache/tomcat/jdbc/pool/DataSourceFactory.java | 22 ++- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 12 ++ .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 30 +++- .../apache/tomcat/jdbc/pool/PoolProperties.java | 19 ++- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 12 ++ .../apache/tomcat/jdbc/pool/mbeans-descriptors.xml | 2 +- .../tomcat/jdbc/pool/TestFacadeAlwaysUsed.java | 163 +++++++++++++++++++ .../jdbc/pool/TestFacadelessProxyIdentity.java | 175 --------------------- .../tomcat/jdbc/pool/TestPoolProperties.java | 16 ++ .../apache/tomcat/jdbc/pool/TestProxyContract.java | 8 +- webapps/docs/changelog.xml | 11 ++ 13 files changed, 298 insertions(+), 202 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index a479b777d9..c0c14fd2f6 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -544,8 +544,16 @@ </p> </attribute> <attribute name="useDisposableConnectionFacade" required="false"> - <p>(boolean) Set this to true if you wish to put a facade on your connection so that it cannot be reused after it has been closed. This prevents a thread holding on to a - reference of a connection it has already called closed on, to execute queries on it. Default value is <code>true</code>. + <p>(boolean) Deprecated. This property no longer has any effect. The pool + always puts a facade on the connections it hands out, so that a + connection cannot be reused after it has been closed: after + <code>close()</code> has been called, all calls on the handle + except <code>close()</code> and <code>isClosed()</code> throw an + exception. Switching the facade off used to leave a handle usable + after close, which allowed it to return or drive the connection of + a later borrower; setting this property to + <code>false</code> is therefore not supported. The property is + still accepted so that existing configurations keep loading. </p> </attribute> <attribute name="logValidationErrors" required="false"> 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 67a3b963f4..b45d4b8cf6 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 @@ -356,15 +356,15 @@ public class ConnectionPool { try { Constructor<?> proxyConstructor = getProxyConstructor(con.getXAConnection() != null); //create the proxy - //TODO possible optimization, keep track if this connection was returned properly, and don't generate a new facade - Connection connection = null; - if (getPoolProperties().getUseDisposableConnectionFacade() ) { - connection = (Connection)proxyConstructor.newInstance(new Object[] { new DisposableConnectionFacade(handler) }); - } else { - connection = (Connection)proxyConstructor.newInstance(new Object[] {handler}); - } - //return the connection - return connection; + //TODO possible optimization, keep track if this connection was returned properly, and don't generate a new handle and facade + // The disposable connection facade is always used; a handle that + // its borrower has closed must stay inert, as the interceptor + // chain below is cached on the pooled connection and re-armed + // for the next borrower. (The useDisposableConnectionFacade + // property that used to switch this off is deprecated and no + // longer has any effect.) + return (Connection)proxyConstructor.newInstance( + new Object[] { new DisposableConnectionFacade(handler) }); }catch (Exception x) { SQLException s = new SQLException(); s.initCause(x); diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java index c949381bb4..ded16d4f93 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java @@ -287,7 +287,9 @@ public class DataSourceFactory implements ObjectFactory { protected static final String PROP_ROLLBACKONRETURN = "rollbackOnReturn"; /** - * Property name for useDisposableConnectionFacade. + * Property name for useDisposableConnectionFacade. The property is + * deprecated and has no effect; the disposable connection facade is + * always used. */ protected static final String PROP_USEDISPOSABLECONNECTIONFACADE = "useDisposableConnectionFacade"; @@ -717,7 +719,8 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_USEDISPOSABLECONNECTIONFACADE); if (value != null) { - poolProperties.setUseDisposableConnectionFacade(Boolean.parseBoolean(value)); + storeDeprecatedUseDisposableConnectionFacade(poolProperties, + Boolean.parseBoolean(value)); } value = properties.getProperty(PROP_LOGVALIDATIONERRORS); @@ -817,4 +820,19 @@ public class DataSourceFactory implements ObjectFactory { return PoolProperties.getProperties(propText,null); } + /** + * Stores the configured value of the deprecated + * useDisposableConnectionFacade property. The property has no effect any + * more, the disposable connection facade is always used; the value is + * stored as configured so that the property keeps showing up the way it + * was set, the same as through any other configuration path. + * @param poolProperties the properties to store the value on + * @param value the configured value + */ + @SuppressWarnings("deprecation") + private static void storeDeprecatedUseDisposableConnectionFacade( + PoolConfiguration poolProperties, boolean value) { + poolProperties.setUseDisposableConnectionFacade(value); + } + } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index 6b66d57631..7fee44776a 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -1226,12 +1226,24 @@ public class DataSourceProxy implements PoolConfiguration { return getPoolProperties().getRollbackOnReturn(); } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public void setUseDisposableConnectionFacade(boolean useDisposableConnectionFacade) { getPoolProperties().setUseDisposableConnectionFacade(useDisposableConnectionFacade); } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public boolean getUseDisposableConnectionFacade() { return getPoolProperties().getUseDisposableConnectionFacade(); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 35dabb6667..3ca87d9831 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -851,17 +851,31 @@ public interface PoolConfiguration { boolean getRollbackOnReturn(); /** - * If set to <code>true</code>, the connection will be wrapped with facade that will disallow the connection to be used after - * {@link java.sql.Connection#close()} is called. If set to <code>true</code>, after {@link java.sql.Connection#close()} all calls except - * {@link java.sql.Connection#close()} and {@link java.sql.Connection#isClosed()} will throw an exception. - * @param useDisposableConnectionFacade <code>true</code> to use a facade - */ + * Stores the value of the deprecated {@code useDisposableConnectionFacade} + * property. The pool always wraps the connection handle in a facade that + * disallows use of the connection after {@link java.sql.Connection#close()} + * has been called, whatever value is stored here. + * @param useDisposableConnectionFacade the configured value, retained but + * without effect + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. Setting it to + * <code>false</code> used to hand out connection handles that + * stayed usable, or worse, able to return and drive the + * connection of a later borrower. + */ + @Deprecated void setUseDisposableConnectionFacade(boolean useDisposableConnectionFacade); /** - * Returns <code>true</code> if this connection pool is configured to use a connection facade to prevent re-use of connection after - * {@link java.sql.Connection#close()} has been invoked - * @return <code>true</code> if {@link java.sql.Connection#close()} has been invoked. + * Returns the value stored for the deprecated + * {@code useDisposableConnectionFacade} property. The pool always wraps the + * connection handle in a facade that disallows use of the connection after + * {@link java.sql.Connection#close()} has been called, whatever value is + * returned here. + * @return the configured value, retained but without effect + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. */ + @Deprecated boolean getUseDisposableConnectionFacade(); /** 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 afb0bd4527..695b03eab4 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 @@ -148,7 +148,12 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl private volatile boolean commitOnReturn = false; /** Whether to rollback on connection return. */ private volatile boolean rollbackOnReturn = false; - /** Whether to use a disposable connection facade. */ + /** + * Whether to use a disposable connection facade. + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ + @Deprecated private volatile boolean useDisposableConnectionFacade = true; /** Whether to log validation errors. */ private volatile boolean logValidationErrors = false; @@ -1180,12 +1185,24 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl return this.rollbackOnReturn; } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public void setUseDisposableConnectionFacade(boolean useDisposableConnectionFacade) { this.useDisposableConnectionFacade = useDisposableConnectionFacade; } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public boolean getUseDisposableConnectionFacade() { return useDisposableConnectionFacade; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java index d1169824cd..cb15d51faf 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java @@ -910,12 +910,24 @@ public class ConnectionPool extends NotificationBroadcasterSupport return getPoolProperties().getRollbackOnReturn(); } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public void setUseDisposableConnectionFacade(boolean useDisposableConnectionFacade) { getPoolProperties().setUseDisposableConnectionFacade(useDisposableConnectionFacade); } + /** + * {@inheritDoc} + * @deprecated The disposable connection facade is always used, so this + * property no longer has any effect. + */ @Override + @Deprecated public boolean getUseDisposableConnectionFacade() { return getPoolProperties().getUseDisposableConnectionFacade(); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml index 677a1b438e..bb78d0097a 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/mbeans-descriptors.xml @@ -300,7 +300,7 @@ writeable="false"/> <attribute name="useDisposableConnectionFacade" - description="If true, connection pool is configured to use a connection facade to prevent re-use of connection after close() has been invoked" + description="Deprecated, no longer has any effect: the pool always uses a connection facade to prevent re-use of connection after close() has been invoked" type="java.lang.Boolean" is="false" writeable="false"/> diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadeAlwaysUsed.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadeAlwaysUsed.java new file mode 100644 index 0000000000..60c2816943 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadeAlwaysUsed.java @@ -0,0 +1,163 @@ +/* + * 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.sql.SQLException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The {@code useDisposableConnectionFacade} property is deprecated and has + * no effect: the pool always wraps the connection handle in a + * {@link DisposableConnectionFacade}. Switching the facade off used to + * expose the cached, re-armed interceptor chain directly, where a handle + * kept from an earlier borrow could return or drive the connection of the + * current borrower. These tests pin the facade against a configuration + * that still asks for it to be switched off. + */ +class TestFacadeAlwaysUsed extends JdbcPoolBaseTest { + + @SuppressWarnings("deprecation") + private DataSource facadelessConfiguration() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setUseDisposableConnectionFacade(false); + return ds; + } + + @Test + void aHandleFromDeprecatedFacadelessConfigurationStaysDisposable() + throws Exception { + DataSource ds = facadelessConfiguration(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 7); + first.close(); // returned to the pool + + Connection second = ds.getConnection(); + Assertions.assertEquals(7, sessionMarker(second).intValue(), + "precondition: the pool re-armed the same physical session"); + + first.close(); // stale double-close from the previous borrower + + // Pre-deprecation, with the facade switched off this stale close ran + // pool.returnConnection() on the connection the second borrower is + // still using. + Assertions.assertEquals(0, ds.getPool().getIdle(), + "the stale close must not hand the current borrower's " + + "connection back to the pool"); + Assertions.assertEquals(1, ds.getPool().getActive()); + + // The current borrower keeps full use of its connection. + setSessionMarker(second, 9); + Assertions.assertEquals(9, sessionMarker(second).intValue()); + + // A stale call is refused instead of driving the live handle. + Assertions.assertThrows(SQLException.class, first::getAutoCommit); + + second.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + } + + @Test + void aReleasedHandleFromDeprecatedFacadelessConfigurationKeepsTheContract() + throws Exception { + DataSource ds = facadelessConfiguration(); + + Connection pooled = ds.getConnection(); + Assertions.assertTrue(pooled.equals(pooled)); + int hash = pooled.hashCode(); + pooled.close(); + + // The disposed facade keeps answering: inert close, closed and + // invalid answers, stable identity. + Assertions.assertTrue(pooled.isClosed(), + "a released handle reports itself closed"); + Assertions.assertFalse(pooled.isValid(1), + "a released handle reports itself invalid"); + Assertions.assertNotNull(pooled.toString(), + "a released handle stays printable for logging"); + 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"); + pooled.close(); // a further close stays a no-op + } + + @Test + void twoBorrowsOverOneSessionAreDistinctHandlesEvenInDeprecatedConfiguration() + throws Exception { DataSource ds = facadelessConfiguration(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 42); + 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.assertNotEquals(first.hashCode(), second.hashCode(), + "two handles must not share an identity hash"); + } + } + + @SuppressWarnings("deprecation") + @Test + void anXaHandleFromDeprecatedFacadelessConfigurationStaysDisposable() + 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(); + setSessionMarker(first, 5); + first.close(); + + Connection second = ds.getConnection(); + Assertions.assertEquals(5, sessionMarker(second).intValue(), + "precondition: the pool re-armed the same physical session"); + + // The forced facade covers the XA proxy class too: a close through + // the stale XA handle must not return the session the current + // borrower is using. + first.close(); + Assertions.assertEquals(0, ds.getPool().getIdle(), + "the stale close must not hand the current borrower's " + + "connection back to the pool"); + Assertions.assertEquals(1, ds.getPool().getActive()); + Assertions.assertTrue(first.isClosed()); + Assertions.assertThrows(SQLException.class, first::getAutoCommit, + "a stale XA handle must not drive the current borrower"); + + second.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + } +} 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 deleted file mode 100644 index fbe30d3067..0000000000 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFacadelessProxyIdentity.java +++ /dev/null @@ -1,175 +0,0 @@ -/* - * 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/TestPoolProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java index c487d15ecd..2016c4f153 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java @@ -112,6 +112,22 @@ class TestPoolProperties { "clearing the configuration must round-trip as well"); } + @Test + @SuppressWarnings("deprecation") + void theDeprecatedFacadePropertyIsStillStoredWhenConfigured() { + Properties attrs = new Properties(); + attrs.setProperty("useDisposableConnectionFacade", "false"); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + // The property has no effect any more, but existing configuration + // must keep loading and keep reading back as it was written. + Assertions.assertFalse(configured.getUseDisposableConnectionFacade(), + "the deprecated value must still round-trip through the " + + "factory path"); + } + @Test void setConnectionPropertiesRoundTripsThroughTheFactory() { Properties attrs = new Properties(); 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 7eb64c67ec..52d1ed131e 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}. 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> + * {@code MultipleCloseTest} and {@code EqualsHashCodeTest}. The same + * contract now also holds for configurations that still ask for the + * (deprecated, no-effect) {@code useDisposableConnectionFacade=false}; + * that is pinned in {@link TestFacadeAlwaysUsed}.</p> */ class TestProxyContract extends JdbcPoolBaseTest { diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index a4d145aa89..d783770d87 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -868,6 +868,17 @@ interceptors exist to provide silently missed exactly the work most likely to be slow. (remm) </fix> + <fix> + Always wrap connection handles in the disposable connection + facade. The <code>useDisposableConnectionFacade</code> property + that could switch the facade off is deprecated and no longer has + any effect; it is still accepted so existing configurations keep + loading. Switching the facade off exposed the pool's cached + interceptor chain directly to the application: a handle kept from + an earlier borrow stayed usable and its close returned – or its + calls drove – the connection a later borrower was still using. + (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
