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 deaa78a36055802943493a96235637d20438cc4d Author: remm <[email protected]> AuthorDate: Wed Oct 28 20:56:21 2026 +0100 Always use the statement facade; deprecate useStatementFacade A statement kept after its connection went back to the pool stayed usable when the pool was configured to install no statement facade. It had nothing marking the end of the borrow, so it went on executing against the physical session the next borrower had taken: it joined that borrower's transaction, saw work that borrower had not committed, and had it rolled back when that borrower rolled back. The same applied to result sets read from such a statement, and Statement.getConnection() reported the driver's own connection, which survives the borrow and bypasses every configured interceptor. The property that allowed this existed only as the escape hatch for Bugzilla 60764, which asked for equals() and hashCode() to be callable on closed statements. That gap is closed in every proxy the pool puts in front of a statement now, facade included, so switching the facade off no longer buys the behaviour it was named for; what it did buy was the hole above. The pool therefore always installs the facade. The property is deprecated, has no effect, but is still accepted and stored so existing configurations keep loading and keep reporting what was set. Tests: TestStatementFacadeAlwaysUsed pins the pooled connection being reported, a statement and a result set retired with the borrow, and the stored value surviving, against a configuration that asks for no facade. The suites that had used that configuration to reach an interceptor's own handle now reach it beneath the facade, so the decorators, the query reports, the statement cache and the statement finalizer keep their contracts pinned at both layers. --- modules/jdbc-pool/doc/jdbc-pool.xml | 23 +- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 10 +- .../apache/tomcat/jdbc/pool/DataSourceFactory.java | 21 +- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 12 + .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 41 ++- .../apache/tomcat/jdbc/pool/PoolProperties.java | 19 +- .../jdbc/pool/interceptor/ResetAbandonedTimer.java | 10 +- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 12 + .../apache/tomcat/jdbc/pool/mbeans-descriptors.xml | 2 +- .../org/apache/tomcat/jdbc/JdbcPoolBaseTest.java | 17 ++ .../tomcat/jdbc/pool/TestPoolProperties.java | 16 + .../jdbc/pool/TestStatementConnectionHandle.java | 16 +- .../jdbc/pool/TestStatementFacadeAlwaysUsed.java | 186 ++++++++++++ .../pool/interceptor/TestQueryReportIdentity.java | 314 +++++++++++-------- .../TestResetAbandonedTimerStreaming.java | 68 ++--- .../pool/interceptor/TestStatementCacheReuse.java | 21 +- .../TestStatementDecoratorIdentity.java | 334 +++++++++++++-------- .../pool/interceptor/TestStatementFinalizer.java | 33 +- webapps/docs/changelog.xml | 10 + 19 files changed, 825 insertions(+), 340 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index a1b532a315..6ea0c54211 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -572,14 +572,21 @@ </p> </attribute> <attribute name="useStatementFacade" required="false"> - <p>(boolean) Set this to true if you wish to wrap statements in order to - enable <code>equals()</code> and <code>hashCode()</code> methods to be - called on the closed statements if any statement proxy is set. - A wrapped statement also reports the pooled connection it was created - from when <code>Statement.getConnection()</code> is called, rather than - the driver's own connection, and stops working, along with the result - sets read from it, once that connection has been returned to the pool. - Default value is <code>true</code>. + <p>(boolean) Deprecated. This property no longer has any effect. The pool + always puts a facade on the statements it hands out, so that + <code>equals()</code>, <code>hashCode()</code> and + <code>toString()</code> can be called on closed statements, + <code>Statement.getConnection()</code> reports the pooled + connection the statement was created from rather than the + driver's own connection, and a statement stops working, + along with the result sets read from it, once that connection + has been returned to the pool. Switching the facade off used + to leave statement and result set handles usable after the + borrow they came from had ended, which allowed a kept handle + to execute on the session the next borrower held; setting + this property to <code>false</code> is therefore not + supported. The property is still accepted so that existing + configurations keep loading. </p> </attribute> 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 1803275c18..075ffbf414 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 @@ -361,9 +361,13 @@ public class ConnectionPool { throw sx; } // setup statement proxy - if (getPoolProperties().getUseStatementFacade()) { - handler = new StatementFacade(handler); - } + // The statement facade is always used; statement and result set + // handles a borrower keeps after returning the connection must stay + // inert, as the interceptor chain below is cached on the pooled + // connection and re-armed for the next borrower. (The + // useStatementFacade property that used to switch this off is + // deprecated and no longer has any effect.) + handler = new StatementFacade(handler); try { Constructor<?> proxyConstructor = getProxyConstructor(con.getXAConnection() != null); //create the proxy 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 d32b545eec..1baad6b7f8 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 @@ -309,7 +309,8 @@ public class DataSourceFactory implements ObjectFactory { protected static final String PROP_IGNOREEXCEPTIONONPRELOAD = "ignoreExceptionOnPreLoad"; /** - * Property name for useStatementFacade. + * Property name for useStatementFacade. The property is deprecated and + * has no effect; the statement facade is always used. */ protected static final String PROP_USESTATEMENTFACADE = "useStatementFacade"; @@ -801,7 +802,8 @@ public class DataSourceFactory implements ObjectFactory { if (value != null) { Boolean b = parseBoolean(PROP_USESTATEMENTFACADE, value); if (b != null) { - poolProperties.setUseStatementFacade(b.booleanValue()); + storeDeprecatedUseStatementFacade(poolProperties, + b.booleanValue()); } } return poolProperties; @@ -948,4 +950,19 @@ public class DataSourceFactory implements ObjectFactory { poolProperties.setUseDisposableConnectionFacade(value); } + /** + * Stores the configured value of the deprecated useStatementFacade + * property. The property has no effect any more, the statement 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 storeDeprecatedUseStatementFacade( + PoolConfiguration poolProperties, boolean value) { + poolProperties.setUseStatementFacade(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 70039f16fa..be3180e93c 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 @@ -1296,12 +1296,24 @@ public class DataSourceProxy implements PoolConfiguration { getPoolProperties().setIgnoreExceptionOnPreLoad(ignoreExceptionOnPreLoad); } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public boolean getUseStatementFacade() { return getPoolProperties().getUseStatementFacade(); } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public void setUseStatementFacade(boolean useStatementFacade) { getPoolProperties().setUseStatementFacade(useStatementFacade); } 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 065442aa8f..8091dd0891 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 @@ -938,22 +938,39 @@ public interface PoolConfiguration { boolean isIgnoreExceptionOnPreLoad(); /** - * Set this to true if you wish to wrap statements in order to enable equals() and hashCode() - * methods to be called on the closed statements if any statement proxy is set. A wrapped - * statement also reports the pooled connection it was created from rather than the driver's - * own connection, and stops working, along with the result sets read from it, once that - * connection has been returned to the pool. - * @param useStatementFacade set to <code>true</code> to wrap statements + * Stores the value of the deprecated {@code useStatementFacade} + * property. The pool always wraps the statements it hands out - and the + * result sets read from them - in a facade, whatever value is stored + * here. That facade answers {@code equals()}, {@code hashCode()} and + * {@code toString()} on closed statements, answers + * {@link java.sql.Statement#getConnection()} with the pooled connection + * the statement was created from, and stops the statement + * and its result sets from working once that connection has been + * returned to the pool. + * @param useStatementFacade the configured value, retained but without + * effect + * @deprecated The statement facade is always used, so this property no + * longer has any effect. Setting it to <code>false</code> + * used to leave statement and result set handles usable + * after the borrow they came from had ended, so a handle + * kept by the application could go on executing on the + * session the next borrower held, and + * {@link java.sql.Statement#getConnection()} returned the + * driver's own connection, which survives the borrow and + * bypasses every configured interceptor. */ + @Deprecated void setUseStatementFacade(boolean useStatementFacade); /** - * Returns <code>true</code> if this connection pool is configured to wrap statements in order - * to enable equals() and hashCode() methods to be called on the closed statements if any - * statement proxy is set. A wrapped statement also reports the pooled connection it was - * created from rather than the driver's own connection, and stops working, along with the - * result sets read from it, once that connection has been returned to the pool. - * @return <code>true</code> if the statements are wrapped + * Returns the value stored for the deprecated {@code useStatementFacade} + * property. The pool always wraps the statements it hands out - and the + * result sets read from them - in a facade, whatever value is returned + * here. + * @return the configured value, retained but without effect + * @deprecated The statement facade is always used, so this property no + * longer has any effect. */ + @Deprecated boolean getUseStatementFacade(); } 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 695b03eab4..74f5b7d017 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 @@ -161,7 +161,12 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl private volatile boolean propagateInterruptState = false; /** Whether to ignore exceptions during pre-load. */ private volatile boolean ignoreExceptionOnPreLoad = false; - /** Whether to use a statement facade. */ + /** + * Whether to use a statement facade. + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ + @Deprecated private volatile boolean useStatementFacade = true; @Override @@ -1237,12 +1242,24 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl this.ignoreExceptionOnPreLoad = ignoreExceptionOnPreLoad; } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public boolean getUseStatementFacade() { return useStatementFacade; } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public void setUseStatementFacade(boolean useStatementFacade) { this.useStatementFacade = useStatementFacade; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java index 9c0636b14f..080dff0d52 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java @@ -43,12 +43,10 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; * Each reset happens when an individual call completes, so a single call * that itself blocks on the database for longer than * {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setRemoveAbandonedTimeout(int)} - * still gets the connection abandoned. When the - * {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setUseStatementFacade(boolean) - * statement facade} is switched off, a result set handle that outlives - * its borrow keeps refreshing the abandoned timer of whoever holds that - * physical connection next; with the facade enabled, as by default, a - * returned borrow is sealed off before that can happen. + * still gets the connection abandoned. The statement facade is always used, + * so a result set handle that outlives its borrow is sealed off before it + * can go on refreshing the abandoned timer of whoever holds that physical + * connection next. * This interceptor is useful for when you have a {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setRemoveAbandonedTimeout(int)} * that is fairly low, and you want to reset the abandoned time each time any operation on the connection is performed * This is useful for batch processing programs that use connections for extensive amount of times. 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 aabec5de21..e0f598b2b7 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 @@ -1055,12 +1055,24 @@ public class ConnectionPool extends NotificationBroadcasterSupport throw new UnsupportedOperationException(); } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public boolean getUseStatementFacade() { return getPoolProperties().getUseStatementFacade(); } + /** + * {@inheritDoc} + * @deprecated The statement facade is always used, so this property no + * longer has any effect. + */ @Override + @Deprecated public void setUseStatementFacade(boolean useStatementFacade) { getPoolProperties().setUseStatementFacade(useStatementFacade); } 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 bb78d0097a..be6ba5a585 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 @@ -334,7 +334,7 @@ writeable="false"/> <attribute name="useStatementFacade" - description="If true, connection pool is configured to wrap statements." + description="Deprecated, no longer has any effect: the pool always uses a statement facade, so statements and result sets cannot be used after the connection they came from is returned" type="java.lang.Boolean" is="false" writeable="false"/> 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 d090336cff..d3c49df9a4 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 @@ -16,6 +16,7 @@ */ package org.apache.tomcat.jdbc; +import java.lang.reflect.Field; import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.DriverManager; @@ -194,6 +195,22 @@ public abstract class JdbcPoolBaseTest { return pooled.unwrap(Statement.class); } + /* + * The object a pooled handle wraps. The statement facade's statement and + * result set handles keep what they wrap in a field named "delegate" on + * their invocation handler, which lets a test assert on the layer below + * the facade - the interceptor's own handle, or the statement the + * interceptors pass on. The handler classes live in the pool package and + * are not nameable from a test elsewhere, so the field is read here. + */ + protected static Object proxiedObject(Object handle) + throws ReflectiveOperationException { + Object handler = Proxy.getInvocationHandler(handle); + Field field = handler.getClass().getDeclaredField("delegate"); + field.setAccessible(true); + return field.get(handler); + } + /* * Number of physical sessions on the database other than the admin * connection this method itself opens - i.e. the connections the 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 2016c4f153..7f04021e43 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 @@ -128,6 +128,22 @@ class TestPoolProperties { + "factory path"); } + @Test + @SuppressWarnings("deprecation") + void theDeprecatedStatementFacadePropertyIsStillStoredWhenConfigured() { + Properties attrs = new Properties(); + attrs.setProperty("useStatementFacade", "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.getUseStatementFacade(), + "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/TestStatementConnectionHandle.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java index fed7ca4736..69f6398fe0 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java @@ -41,8 +41,8 @@ import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; * ({@link StatementFinalizer}'s close-on-return, a statement cache).</p> * * <p>{@link StatementDecoratorInterceptor} has always answered this call - * with the pooled handle; {@link StatementFacade}, which is on by default, - * did not.</p> + * with the pooled handle, and so does the statement facade that is now + * always installed.</p> */ class TestStatementConnectionHandle extends JdbcPoolBaseTest { @@ -188,6 +188,7 @@ class TestStatementConnectionHandle extends JdbcPoolBaseTest { * answer does not depend on an unrelated interceptor being configured. */ @Test + @SuppressWarnings("deprecation") void theDecoratorAndTheFacadeAnswerTheSameWay() throws Exception { DataSource ds = newDataSource(); ds.getPoolProperties().setJdbcInterceptors( @@ -200,6 +201,10 @@ class TestStatementConnectionHandle extends JdbcPoolBaseTest { "with the decorator configured the reported connection must stay the pooled handle"); } + // The deprecated value asks for the facade not to be installed; it + // no longer removes it, so this handle is the facade's proxy and the + // decorator's own proxy sits right underneath it. Both layers must + // give the same answer. DataSource unfacaded = newDataSource(); unfacaded.getPoolProperties().setUseStatementFacade(false); unfacaded.getPoolProperties().setJdbcInterceptors( @@ -208,7 +213,12 @@ class TestStatementConnectionHandle extends JdbcPoolBaseTest { try (Connection pooled = unfacaded.getConnection(); Statement st = pooled.createStatement()) { Assertions.assertSame(pooled, st.getConnection(), - "the decorator alone must keep answering with the pooled handle"); + "the application handle must report the pooled handle even with the " + + "deprecated value set"); + try (Statement inner = (Statement) proxiedObject(st)) { + Assertions.assertSame(pooled, inner.getConnection(), + "the decorator handle beneath the facade must answer the same way"); + } } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementFacadeAlwaysUsed.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementFacadeAlwaysUsed.java new file mode 100644 index 0000000000..5e6f061418 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementFacadeAlwaysUsed.java @@ -0,0 +1,186 @@ +/* + * 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.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The {@code useStatementFacade} property is deprecated and has no effect: + * the pool always wraps the statements it hands out - and the result sets + * read from them - in a {@link StatementFacade}. Switching the facade off + * used to leave a handle kept after the borrow ended fully usable: it + * executed against the physical session the next borrower held, and + * {@code getConnection()} reported the driver's own connection, which + * survives the borrow and bypasses every configured interceptor. These + * tests pin the facade against a configuration that still asks for it to be + * switched off. + */ +class TestStatementFacadeAlwaysUsed extends JdbcPoolBaseTest { + + /* One borrow at a time over one idle session, so the next borrow + * demonstrably gets the very session the retired handle was made on. */ + @SuppressWarnings("deprecation") + private DataSource facadelessConfiguration() throws SQLException { + DataSource ds = newDataSource(); + ds.getPoolProperties().setUseStatementFacade(false); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + return ds; + } + + @Test + void aStatementFromDeprecatedFacadelessConfigurationReportsThePooledHandle() + throws Exception { + DataSource ds = facadelessConfiguration(); + + try (Connection pooled = ds.getConnection()) { + try (Statement plain = pooled.createStatement()) { + Assertions.assertSame(pooled, plain.getConnection(), + "the facadeless request must not resurrect the driver's " + + "connection behind getConnection()"); + } + try (Statement prepared = pooled.prepareStatement("SELECT 1")) { + Assertions.assertSame(pooled, prepared.getConnection(), + "a prepared statement must report the pooled handle as well"); + } + // The retired handle of the very first test below would otherwise + // still be able to drive the session through this leak. + try (Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertSame(pooled, rs.getStatement().getConnection(), + "the connection reached through a result set must be the pooled handle"); + } + } + } + + @Test + void aStatementFromDeprecatedFacadelessConfigurationIsRetiredWithTheBorrow() + throws Exception { + DataSource ds = facadelessConfiguration(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 7); + Statement survivor = first.createStatement(); + 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"); + + // Pre-deprecation, with the facade switched off this handle simply + // executed: on the session the second borrower is now using. Writing + // the session marker is what makes such an intrusion observable. + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> survivor.execute("SET @POOL_TEST_MARKER = 999"), + "a statement whose borrow ended must not run against the " + + "next borrower's session"); + Assertions.assertTrue(thrown.getMessage().contains( + "The pooled connection this statement was created from has " + + "been returned to the pool."), + "the failure must be the facade's wording, but said: " + + thrown.getMessage()); + Assertions.assertEquals(7, sessionMarker(second).intValue(), + "the next borrower's session must be untouched"); + + Assertions.assertTrue(survivor.isClosed(), + "a retired handle reports itself closed"); + Assertions.assertTrue(survivor.equals(survivor), + "equals stays answerable on the retired handle"); + int hash = survivor.hashCode(); + Assertions.assertEquals(hash, survivor.hashCode(), + "hashCode stays a stable value on the retired handle"); + + // Closing such a handle twice over stays the harmless no-op a closed + // statement's close() is. + survivor.close(); + survivor.close(); + + // The second borrower keeps full use of its session. + try (Statement st = second.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the current borrower must get working handles"); + } + second.close(); + } + + @Test + void aResultSetFromDeprecatedFacadelessConfigurationIsSealedWithTheBorrow() + throws Exception { + DataSource ds = facadelessConfiguration(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 8); + Statement st = first.createStatement(); + ResultSet survivor = st.executeQuery("SELECT X FROM SYSTEM_RANGE(1,10)"); + Assertions.assertTrue(survivor.next(), "the cursor is live during the borrow"); + first.close(); + + Connection second = ds.getConnection(); + Assertions.assertEquals(8, sessionMarker(second).intValue(), + "precondition: the pool re-armed the same physical session"); + + Assertions.assertTrue(survivor.isClosed(), + "a result set whose borrow ended must report itself closed"); + SQLException thrown = Assertions.assertThrows(SQLException.class, survivor::next, + "a result set whose borrow ended must not read the next " + + "borrower's session"); + Assertions.assertTrue(thrown.getMessage().contains( + "The pooled connection this result set was read from has " + + "been returned to the pool."), + "the failure must be the facade's wording, but said: " + + thrown.getMessage()); + Assertions.assertEquals(8, sessionMarker(second).intValue(), + "the next borrower's session must be untouched"); + survivor.close(); // the no-op a closed result set's close() is + + try (Statement probe = second.createStatement(); + ResultSet rs = probe.executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), + "the current borrower must get working handles"); + } + second.close(); + } + + @SuppressWarnings("deprecation") + @Test + void theDeprecatedValueIsStillStoredAndReported() throws Exception { + DataSource ds = facadelessConfiguration(); + try (Connection pooled = ds.getConnection()) { + Assertions.assertFalse(pooled.isClosed(), + "precondition: the pool serves connections from a " + + "configuration that asks for no facade"); + } + + // The property has no effect any more, but the configuration must + // keep reading back as it was written - through the data source + // proxy and through the pool properties behind it. + Assertions.assertFalse(ds.getUseStatementFacade(), + "the data source must keep reporting the configured value"); + Assertions.assertFalse(ds.getPoolProperties().getUseStatementFacade(), + "the pool properties must keep storing the configured value"); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportIdentity.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportIdentity.java index 95d45d44f8..2fec8c2e3a 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportIdentity.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryReportIdentity.java @@ -47,56 +47,75 @@ import org.apache.tomcat.jdbc.pool.DataSource; * to answer, so every one of these methods surfaced as an * {@link UndeclaredThrowableException} wrapping "Statement closed.". * - * <p>With the (default) statement facade enabled it sits outermost and - * answers these methods itself, which masks the gap; these tests pin the - * unfacaded configuration - {@code useStatementFacade=false} is a documented - * configuration - and that both configurations answer identically.</p> + * <p>The pool always installs its statement facade outermost (the + * {@code useStatementFacade} property that used to switch it off is + * deprecated and has no effect), and the facade answers these methods on the + * handle the application holds. The interceptor's own proxies stay live + * inner layers under the facade's handles, so the tests reach them through + * the facade's delegate and pin the contract at both layers.</p> */ class TestQueryReportIdentity extends JdbcPoolBaseTest { - private DataSource queryReportPool(String interceptors, boolean useStatementFacade) - throws SQLException { + private DataSource queryReportPool(String interceptors) throws SQLException { DataSource ds = newDataSource(); - ds.getPoolProperties().setUseStatementFacade(useStatementFacade); ds.getPoolProperties().setJdbcInterceptors(interceptors); ds.createPool(); return ds; } - private DataSource slowQueryPool(boolean useStatementFacade) throws SQLException { - return queryReportPool(SlowQueryReport.class.getSimpleName(), useStatementFacade); + private DataSource slowQueryPool() throws SQLException { + return queryReportPool(SlowQueryReport.class.getSimpleName()); + } + + /* The interceptor's own statement handle behind the facade's. */ + private static Statement statementBehindFacade(Statement facadeHandle) + throws ReflectiveOperationException { + return (Statement) proxiedObject(facadeHandle); + } + + /* The contract of an open statement handle, for one layer. */ + private static void assertOpenStatementHandle(Statement handle, + Connection pooled, String layer) throws SQLException { + Assertions.assertTrue(handle.equals(handle), + layer + ": a statement handle must be equal to itself"); + Assertions.assertFalse(handle.equals(null), + layer + ": a statement handle is not equal to null"); + Assertions.assertFalse(handle.equals(pooled), + layer + ": a statement handle is not equal to its connection"); + Assertions.assertFalse(handle.equals(new Object()), + layer + ": a statement handle is not equal to a foreign object"); + // the driver object underneath is not the handle: an equality the + // driver answers is the driver's own identity, not the identity of + // the handle above it + Assertions.assertFalse(handle.equals(driverStatement(handle)), + layer + ": a statement handle is not equal to the driver object below it"); + int hash = handle.hashCode(); + Assertions.assertEquals(hash, handle.hashCode(), + layer + ": hashCode must be a stable value of the handle itself"); + Set<Statement> seen = new HashSet<>(); + seen.add(handle); + Assertions.assertTrue(seen.contains(handle), + layer + ": hashCode must agree with equals"); } @Test void aQueryReportStatementHandleIsEqualOnlyToItselfWhileOpen() throws Exception { - DataSource ds = slowQueryPool(false); + DataSource ds = slowQueryPool(); try (Connection pooled = ds.getConnection()) { try (Statement st = pooled.createStatement()) { - Assertions.assertTrue(st.equals(st), - "a statement handle must be equal to itself"); - Assertions.assertFalse(st.equals(null), - "a statement handle is not equal to null"); - Assertions.assertFalse(st.equals(pooled), - "a statement handle is not equal to its connection"); - Assertions.assertFalse(st.equals(new Object()), - "a statement handle is not equal to a foreign object"); - // the driver object underneath is not the handle: an equality - // the driver answers is the driver's own identity, not the - // identity of the handle the application holds - Assertions.assertFalse(st.equals(driverStatement(st)), - "a statement handle is not equal to the driver object below it"); + assertOpenStatementHandle(st, pooled, "the statement facade"); + assertOpenStatementHandle(statementBehindFacade(st), pooled, + "the query report proxy"); try (Statement other = pooled.createStatement()) { Assertions.assertFalse(st.equals(other), - "two statements are two handles"); + "two statements are two handles at the facade layer"); Assertions.assertFalse(other.equals(st)); + Assertions.assertFalse(statementBehindFacade(st).equals(other), + "and the proxy of one statement is not the handle of another"); + Assertions.assertFalse(statementBehindFacade(st) + .equals(statementBehindFacade(other)), + "two statements are two handles at the interceptor layer"); } - int hash = st.hashCode(); - Assertions.assertEquals(hash, st.hashCode(), - "hashCode must be a stable value of the handle itself"); - Set<Statement> seen = new HashSet<>(); - seen.add(st); - Assertions.assertTrue(seen.contains(st), - "hashCode must agree with equals"); } try (PreparedStatement pst = pooled.prepareStatement("SELECT 1")) { Assertions.assertTrue(pst.equals(pst), @@ -105,66 +124,90 @@ class TestQueryReportIdentity extends JdbcPoolBaseTest { "two prepared statements are two handles"); Assertions.assertEquals(pst.hashCode(), pst.hashCode(), "hashCode must be a stable value of the handle itself"); + Statement inner = statementBehindFacade(pst); + Assertions.assertTrue(inner.equals(inner), + "the interceptor's prepared statement proxy is equal to itself"); } } } @Test void aClosedQueryReportHandleKeepsAnsweringWithItsOwnIdentity() throws Exception { - DataSource ds = slowQueryPool(false); - Connection pooled = ds.getConnection(); - try { + DataSource ds = slowQueryPool(); + try (Connection pooled = ds.getConnection()) { Statement st = pooled.createStatement(); - int hash = st.hashCode(); - Map<Statement, String> byHandle = new HashMap<>(); - byHandle.put(st, "the handle"); + Statement reportHandle = statementBehindFacade(st); + int facadeHash = st.hashCode(); + int reportHash = reportHandle.hashCode(); + Map<Statement, String> byFacadeHandle = new HashMap<>(); + byFacadeHandle.put(st, "the handle"); + Map<Statement, String> byReportHandle = new HashMap<>(); + byReportHandle.put(reportHandle, "the handle"); st.close(); - // None of these three methods declares a checked exception, so a - // throwing handler cannot reach the application as a SQLException - // - the proxy machinery wraps it. All three answers must simply - // be values. - Assertions.assertDoesNotThrow(st::hashCode, - "hashCode on a closed handle must return, not throw"); - Assertions.assertEquals(hash, st.hashCode(), - "closing must not change the identity of a handle"); - Assertions.assertTrue(st.equals(st), - "closing must not break reflexivity of a handle"); - Assertions.assertFalse(st.equals(pooled), - "a closed handle is still equal to nothing but itself"); - Assertions.assertDoesNotThrow(() -> String.valueOf(st), - "toString on a closed handle must return, not throw"); - // the shapes that actually reach the defect in practice: hashing a - // stale handle for a collection, and stringifying one for a log - // line or an assertion message - try { - Assertions.assertEquals("the handle", byHandle.get(st), - "a closed handle must still find its entry in a map"); - Assertions.assertEquals(hash, st.hashCode()); - Assertions.assertTrue(String.valueOf(st).contains("Query=")); - } catch (UndeclaredThrowableException e) { - Assertions.fail("a closed statement handle must not surface " - + "UndeclaredThrowableException: " + e.getCause()); + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the query report proxy"; + Statement handle = layer == 0 ? st : reportHandle; + int hash = layer == 0 ? facadeHash : reportHash; + Map<Statement, String> byHandle = layer == 0 ? byFacadeHandle : byReportHandle; + + // None of these three methods declares a checked exception, + // so a throwing handler cannot reach the application as a + // SQLException - the proxy machinery wraps it. All three + // answers must simply be values. + Assertions.assertDoesNotThrow(handle::hashCode, + name + ": hashCode on a closed handle must return, not throw"); + Assertions.assertEquals(hash, handle.hashCode(), + name + ": closing must not change the identity of a handle"); + Assertions.assertTrue(handle.equals(handle), + name + ": closing must not break reflexivity of a handle"); + Assertions.assertFalse(handle.equals(pooled), + name + ": a closed handle is still equal to nothing but itself"); + Assertions.assertDoesNotThrow(() -> String.valueOf(handle), + name + ": toString on a closed handle must return, not throw"); + // the shapes that actually reach the defect in practice: + // hashing a stale handle for a collection, and stringifying + // one for a log line or an assertion message + try { + Assertions.assertEquals("the handle", byHandle.get(handle), + name + ": a closed handle must still find its entry in a map"); + Assertions.assertEquals(hash, handle.hashCode()); + Assertions.assertTrue(String.valueOf(handle).contains("Query=")); + } catch (UndeclaredThrowableException e) { + Assertions.fail(name + ": a closed statement handle must not surface " + + "UndeclaredThrowableException: " + e.getCause()); + } } - } finally { - pooled.close(); } } @Test void aQueryReportHandleDescribesItselfRatherThanTheDriverBelowIt() throws Exception { - DataSource ds = slowQueryPool(false); + DataSource ds = slowQueryPool(); try (Connection pooled = ds.getConnection()) { PreparedStatement pst = pooled.prepareStatement("SELECT 1"); - String open = Assertions.assertDoesNotThrow(pst::toString, + // The shape with the interceptor's name belongs to the query + // report proxy below the facade; the facade's own handle + // describes the facade instead. Neither may leak the driver. + Statement reportHandle = statementBehindFacade(pst); + + String open = Assertions.assertDoesNotThrow(reportHandle::toString, "toString on an open handle must return, not throw"); Assertions.assertTrue(open.contains(SlowQueryReport.class.getName()), "the handle must say which interceptor produced it, was: " + open); Assertions.assertTrue(open.contains("SELECT 1"), "the handle must name its query, was: " + open); + String facadeSide = Assertions.assertDoesNotThrow(pst::toString, + "toString on the application's handle must return, not throw"); + Assertions.assertTrue(facadeSide.contains("StatementFacade"), + "the facade handle must say what it is, was: " + facadeSide); + Assertions.assertTrue(facadeSide.contains("SELECT 1"), + "and name its query, was: " + facadeSide); + pst.close(); - String closed = Assertions.assertDoesNotThrow(pst::toString, + String closed = Assertions.assertDoesNotThrow(reportHandle::toString, "toString on a closed handle must return, not throw"); Assertions.assertTrue(closed.contains("SELECT 1"), "a closed handle must still name its query, was: " + closed); @@ -180,74 +223,99 @@ class TestQueryReportIdentity extends JdbcPoolBaseTest { */ @Test void aClosedQueryReportHandleStillRefusesTheWorkItWasCreatedFor() throws Exception { - DataSource ds = slowQueryPool(false); - Connection pooled = ds.getConnection(); - try { + DataSource ds = slowQueryPool(); + try (Connection pooled = ds.getConnection()) { Statement st = pooled.createStatement(); + Statement reportHandle = statementBehindFacade(st); st.close(); - SQLException onExecute = Assertions.assertThrows(SQLException.class, - () -> st.execute("SELECT 1")); - Assertions.assertTrue(onExecute.getMessage().contains("closed"), - "the closed contract still reports a closed statement: " - + onExecute.getMessage()); - Assertions.assertTrue(st.isClosed(), - "isClosed must report the closed handle"); - Assertions.assertThrows(SQLException.class, st::getConnection, - "a closed handle still reports its connection as gone"); - Assertions.assertThrows(SQLException.class, () -> st.unwrap(Statement.class), - "unwrap on a closed handle still fails"); - Assertions.assertThrows(SQLException.class, () -> st.isWrapperFor(Statement.class), - "isWrapperFor on a closed handle still fails"); - Assertions.assertThrows(SQLException.class, () -> st.setFetchSize(1), - "a business method on a closed handle still fails"); - } finally { - pooled.close(); + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the query report proxy"; + Statement handle = layer == 0 ? st : reportHandle; + + SQLException onExecute = Assertions.assertThrows(SQLException.class, + () -> handle.execute("SELECT 1"), + name + ": a closed handle must refuse work"); + Assertions.assertTrue(onExecute.getMessage().contains("closed"), + name + ": the closed contract still reports a closed statement: " + + onExecute.getMessage()); + Assertions.assertTrue(handle.isClosed(), + name + ": isClosed must report the closed handle"); + Assertions.assertThrows(SQLException.class, handle::getConnection, + name + ": a closed handle still reports its connection as gone"); + Assertions.assertThrows(SQLException.class, () -> handle.unwrap(Statement.class), + name + ": unwrap on a closed handle still fails"); + Assertions.assertThrows(SQLException.class, + () -> handle.isWrapperFor(Statement.class), + name + ": isWrapperFor on a closed handle still fails"); + Assertions.assertThrows(SQLException.class, () -> handle.setFetchSize(1), + name + ": a business method on a closed handle still fails"); + } } } /* - * The facade absorbs the Object methods from outside the interceptor, and - * so does the abandoned timer's own proxy; neither configuration and - * neither interceptor may change an answer the application gets. + * The facade always absorbs the Object methods from outside the + * interceptor, and the abandoned timer wraps once more on top of the + * query report proxy; neither interceptor and neither layer may change + * an answer the contract requires. */ + private static String layerName(int layer) { + return layer == 0 ? "facade handle" : "interceptor handle"; + } + @Test - void everyConfigurationAnswersTheSameWay() throws Exception { + void everyQueryReportInterceptorAnswersTheSameWayAtBothLayers() throws Exception { String[] interceptors = { SlowQueryReport.class.getSimpleName(), ResetAbandonedTimer.class.getSimpleName() }; for (String interceptor : interceptors) { - for (boolean facade : new boolean[] { false, true }) { - String config = interceptor + " " + (facade ? "with" : "without"); - DataSource ds = queryReportPool(interceptor, facade); - try (Connection pooled = ds.getConnection()) { - Statement st = pooled.createStatement(); - try (PreparedStatement pst = pooled.prepareStatement("SELECT 1")) { - Assertions.assertTrue(pst.equals(pst), - config + " the facade a prepared handle is equal to itself"); - Assertions.assertFalse(pst.equals(st), - config + " the facade two statements are two handles"); - } - - Assertions.assertTrue(st.equals(st), - config + " the facade a statement handle is equal to itself"); - Assertions.assertFalse(st.equals(null), - config + " the facade equals(null) is false"); - int hash = st.hashCode(); - Map<Statement, String> byHandle = new HashMap<>(); - byHandle.put(st, "the handle"); - - st.close(); - - Assertions.assertTrue(st.equals(st), - config + " the facade a closed handle stays equal to itself"); - Assertions.assertFalse(st.equals(pooled), - config + " the facade a closed handle is equal to nothing else"); - Assertions.assertEquals(hash, st.hashCode(), - config + " the facade the hash survives the close"); - Assertions.assertEquals("the handle", byHandle.get(st), - config + " the facade a closed handle still hashes into its map"); - Assertions.assertNotNull(String.valueOf(st), - config + " the facade the closed handle still describes itself"); + DataSource ds = queryReportPool(interceptor); + try (Connection pooled = ds.getConnection()) { + Statement st = pooled.createStatement(); + Statement inner = statementBehindFacade(st); + try (PreparedStatement pst = pooled.prepareStatement("SELECT 1")) { + Assertions.assertTrue(pst.equals(pst), + interceptor + ": a prepared handle is equal to itself"); + Assertions.assertFalse(pst.equals(st), + interceptor + ": two statements are two handles"); + Statement innerPst = statementBehindFacade(pst); + Assertions.assertTrue(innerPst.equals(innerPst), + interceptor + ": the interceptor's own prepared proxy is equal to itself"); + Assertions.assertFalse(innerPst.equals(inner), + interceptor + ": two proxies are two handles below the facade"); + } + + Statement[] handles = { st, inner }; + int[] hashes = new int[2]; + @SuppressWarnings("unchecked") + Map<Statement, String>[] byHandle = new Map[] { + new HashMap<Statement, String>(), new HashMap<Statement, String>() }; + for (int layer = 0; layer < 2; layer++) { + Assertions.assertTrue(handles[layer].equals(handles[layer]), + interceptor + " " + layerName(layer) + ": a statement handle is equal to itself"); + Assertions.assertFalse(handles[layer].equals(null), + interceptor + " " + layerName(layer) + ": equals(null) is false"); + hashes[layer] = handles[layer].hashCode(); + byHandle[layer].put(handles[layer], "the handle"); + } + + st.close(); // both layers then face the same closed state + + for (int layer = 0; layer < 2; layer++) { + String name = interceptor + " " + layerName(layer); + Statement handle = handles[layer]; + + Assertions.assertTrue(handle.equals(handle), + name + ": a closed handle stays equal to itself"); + Assertions.assertFalse(handle.equals(pooled), + name + ": a closed handle is equal to nothing else"); + Assertions.assertEquals(hashes[layer], handle.hashCode(), + name + ": the hash survives the close"); + Assertions.assertEquals("the handle", byHandle[layer].get(handle), + name + ": a closed handle still hashes into its map"); + Assertions.assertNotNull(String.valueOf(handle), + name + ": the closed handle still describes itself"); } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerStreaming.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerStreaming.java index e0df35edf2..6945021cf6 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerStreaming.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerStreaming.java @@ -40,8 +40,14 @@ import org.apache.tomcat.jdbc.pool.DataSource; * assert which of the two physical sessions the cleaner killed. The * composition test pins that the result set handles the interceptor * hands out stay usable, obey the identity contract of a handle, and - * compose with the statement facade on every access path that yields a - * result set. + * compose with the (always installed) statement facade on every access + * path that yields a result set. + * + * <p>The pool under test still configures the deprecated + * {@code useStatementFacade=false}, so the handle composition asserted below + * is the one an application gets whatever it asks for: the facade is always + * installed. That the property no longer has an effect is what + * {@code TestStatementFacadeAlwaysUsed} pins down.</p> */ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { @@ -68,14 +74,19 @@ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { */ private static final int MIN_FAILURE_CALLS = 5; - private DataSource streamingPool(boolean useStatementFacade) throws SQLException { + @SuppressWarnings("deprecation") + private DataSource streamingPool() throws SQLException { DataSource ds = newDataSource(); ds.getPoolProperties().setJdbcInterceptors( "org.apache.tomcat.jdbc.pool.interceptor.ResetAbandonedTimer"); ds.getPoolProperties().setRemoveAbandoned(true); ds.getPoolProperties().setRemoveAbandonedTimeout(1); // seconds ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(100); - ds.getPoolProperties().setUseStatementFacade(useStatementFacade); + // Deprecated and without effect: the statement facade is always + // used. Running the scenarios on a pool that still asks for it to + // be switched off pins that the request can no longer take the + // interceptor's keep-alive proxies away. + ds.getPoolProperties().setUseStatementFacade(false); ds.createPool(); pool = ds; return ds; @@ -156,9 +167,9 @@ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { return rows; } - private void assertStreamingKeepsConnectionAlive(boolean useStatementFacade) throws Exception { + private void assertStreamingKeepsConnectionAlive() throws Exception { awaitSessions(0); - DataSource ds = streamingPool(useStatementFacade); + DataSource ds = streamingPool(); Connection busy = borrowBusyWithIdleControl(ds); int rows = streamSlowly(busy); @@ -172,23 +183,15 @@ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { * An application iterating a result set is active for as long as it * reads, not only until the query returns the cursor position. The * abandoned timer must be reset by the fetch traffic driven by - * ResultSet.next() - through the statement facade that wraps the - * interceptor's result set proxy by default. + * ResultSet.next() - through the always-installed statement facade + * wrapping the interceptor's result set proxy. The pool under test + * still asks for the facade to be switched off through the deprecated + * property, so this also pins that the keep-alive proxies can no + * longer be taken away that way. */ @Test void streamingAResultSetKeepsConnectionAlive() throws Exception { - assertStreamingKeepsConnectionAlive(true); - } - - /* - * The result set keep-alive belongs to the interceptor itself, not - * to the statement facade layered around it: with the facade - * switched off, fetching rows must keep the connection alive just - * the same. - */ - @Test - void streamingKeepsConnectionAliveWithoutStatementFacade() throws Exception { - assertStreamingKeepsConnectionAlive(false); + assertStreamingKeepsConnectionAlive(); } /* @@ -203,7 +206,7 @@ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { @Test void failedResultSetReadsKeepConnectionAlive() throws Exception { awaitSessions(0); - DataSource ds = streamingPool(true); + DataSource ds = streamingPool(); Connection busy = borrowBusyWithIdleControl(ds); int failures = 0; @@ -234,25 +237,22 @@ class TestResetAbandonedTimerStreaming extends JdbcPoolBaseTest { } /* - * The result set handle the interceptor inserts under (or, without - * the facade, in place of) the facade's own handle must behave: it - * must yield the rows, obey the identity contract of a handle (a - * handle is equal to itself and to nothing else), hand out a working - * statement through getStatement() - the handle in use by this - * interceptor, which an interceptor configured around it may have - * wrapped further - close cleanly, and do so on all three paths that - * produce a result set - executeQuery, getResultSet and - * getGeneratedKeys - with the facade composing around it. + * The result set handle the interceptor inserts beneath the facade's + * own handle must behave: it must yield the rows, obey the identity + * contract of a handle (a handle is equal to itself and to nothing + * else), hand out a working statement through getStatement() - the + * facade answers that call with the application's own statement + * handle - close cleanly, and do so on all three paths that produce a + * result set - executeQuery, getResultSet and getGeneratedKeys. */ @Test void resultSetsOnEveryAccessPathStayUsableAndHandleStable() throws Exception { awaitSessions(0); - assertResultSetHandlesCompose(true); - assertResultSetHandlesCompose(false); + assertResultSetHandlesCompose(); } - private void assertResultSetHandlesCompose(boolean useStatementFacade) throws SQLException { - DataSource ds = streamingPool(useStatementFacade); + private void assertResultSetHandlesCompose() throws SQLException { + DataSource ds = streamingPool(); Connection con = ds.getConnection(); String table = uniqueTable(); try (Statement st = con.createStatement()) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java index 98d66a512b..d4068995da 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java @@ -118,10 +118,6 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { @Test void aRecycledStatementReportsTheBorrowerItIsHandedTo() throws Exception { DataSource ds = cachedPool("prepared=true,max=5", 1); - // Without the statement facade the cache's own proxy is what the - // application holds; the facade answers getConnection() itself and - // so masks what the proxy behind it carries. - ds.getPoolProperties().setUseStatementFacade(false); String table = uniqueTable(); ds.createPool(); try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { @@ -149,15 +145,24 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { Assertions.assertEquals(0, cacheOf(second).getCacheSizePerConnection(), "the statement must have come out of the cache"); + // The application handle is the facade's proxy, which answers + // from the current borrow (pinned separately by + // aRecycledStatementBehindTheDefaultFacadeReportsTheCurrentBorrower). + // Beneath it sits the cache's own proxy, and that is the object + // the cache must have rebound to the new borrower. Connection reported = reused.getConnection(); Assertions.assertSame(second, reported, "the recycled statement must report the borrower it is handed to, " + "not the borrower that closed it"); - Assertions.assertFalse(reported.isClosed(), - "the reported connection must be the live handle of the current borrow"); - try (Statement probe = reported.createStatement()) { + Statement inner = (Statement) proxiedObject(reused); + Assertions.assertSame(second, inner.getConnection(), + "the cache's own proxy must have been rebound to the new borrower"); + Connection bound = inner.getConnection(); + Assertions.assertFalse(bound.isClosed(), + "the bound connection must be the live handle of the current borrow"); + try (Statement probe = bound.createStatement()) { Assertions.assertTrue(probe.execute("SELECT 1"), - "the reported connection must serve the current borrower"); + "the bound connection must serve the current borrower"); } reused.setInt(1, 2); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorIdentity.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorIdentity.java index 25523110ec..f02daedea9 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorIdentity.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorIdentity.java @@ -43,118 +43,171 @@ import org.apache.tomcat.jdbc.pool.DataSource; * no delegate left, so every Object method surfaced as an * {@link UndeclaredThrowableException} wrapping "Statement closed.". * - * <p>With the (default) statement facade enabled it sits outermost and - * answers these methods itself, which masks the gap; these tests pin the - * unfacaded configuration - {@code useStatementFacade=false} is a documented - * configuration - and that both configurations answer identically.</p> + * <p>The pool always installs its statement facade outermost (the + * {@code useStatementFacade} property that used to switch it off is + * deprecated and has no effect), and the facade answers these methods on the + * handle the application holds. The decorator's own proxies stay live inner + * layers under the facade's handles - the facade wraps what the interceptor + * chain returns - so the tests reach them through the facade's delegate and + * pin the contract at both layers.</p> */ class TestStatementDecoratorIdentity extends JdbcPoolBaseTest { - private DataSource decoratorPool(boolean useStatementFacade) throws SQLException { + private DataSource decoratorPool() throws SQLException { DataSource ds = newDataSource(); - ds.getPoolProperties().setUseStatementFacade(useStatementFacade); ds.getPoolProperties().setJdbcInterceptors( StatementDecoratorInterceptor.class.getSimpleName()); ds.createPool(); return ds; } + /* The interceptor's own statement handle behind the facade's. */ + private static Statement statementBehindFacade(Statement facadeHandle) + throws ReflectiveOperationException { + return (Statement) proxiedObject(facadeHandle); + } + + /* The interceptor's own result set handle behind the facade's. */ + private static ResultSet resultSetBehindFacade(ResultSet facadeHandle) + throws ReflectiveOperationException { + return (ResultSet) proxiedObject(facadeHandle); + } + + /* The contract of an open statement handle, for one layer. */ + private static void assertOpenStatementHandle(Statement handle, + Statement other, Connection pooled, String layer) { + Assertions.assertTrue(handle.equals(handle), + layer + ": a statement handle must be equal to itself"); + Assertions.assertFalse(handle.equals(null), + layer + ": a statement handle is not equal to null"); + Assertions.assertFalse(handle.equals(pooled), + layer + ": a statement handle is not equal to its connection"); + Assertions.assertFalse(handle.equals(other), + layer + ": two statements are two handles"); + Assertions.assertFalse(other.equals(handle), + layer + ": and not in the other direction either"); + int hash = handle.hashCode(); + Assertions.assertEquals(hash, handle.hashCode(), + layer + ": hashCode must be a stable value of the handle itself"); + Set<Statement> seen = new HashSet<>(); + seen.add(handle); + Assertions.assertTrue(seen.contains(handle), + layer + ": hashCode must agree with equals"); + } + + /* The contract of a closed statement handle, for one layer. */ + private static void assertClosedStatementHandle(Statement handle, + int hashBeforeClose, Connection pooled, String layer) { + // Neither method declares a checked exception, so a throwing handler + // cannot reach the application as a SQLException - the proxy layer + // wraps it. Both answers must simply be values. + Assertions.assertDoesNotThrow(handle::hashCode, + layer + ": hashCode on a closed handle must return, not throw"); + Assertions.assertEquals(hashBeforeClose, handle.hashCode(), + layer + ": closing must not change the identity of a handle"); + Assertions.assertTrue(handle.equals(handle), + layer + ": closing must not break reflexivity of a handle"); + Assertions.assertFalse(handle.equals(pooled), + layer + ": a closed handle is still equal to nothing but itself"); + try { + handle.hashCode(); + } catch (UndeclaredThrowableException e) { + Assertions.fail(layer + ": hashCode on a closed handle must not " + + "surface as UndeclaredThrowableException: " + e.getCause()); + } + } + @Test void aStatementHandleIsEqualOnlyToItselfWhileOpen() throws Exception { - DataSource ds = decoratorPool(false); + DataSource ds = decoratorPool(); try (Connection pooled = ds.getConnection()) { - try (Statement st = pooled.createStatement()) { - Assertions.assertTrue(st.equals(st), - "a statement handle must be equal to itself"); - Assertions.assertFalse(st.equals(null), - "a statement handle is not equal to null"); - Assertions.assertFalse(st.equals(pooled), - "a statement handle is not equal to its connection"); - try (Statement other = pooled.createStatement()) { - Assertions.assertFalse(st.equals(other), - "two statements are two handles"); - Assertions.assertFalse(other.equals(st)); - } - int hash = st.hashCode(); - Assertions.assertEquals(hash, st.hashCode(), - "hashCode must be a stable value of the handle itself"); - Set<Statement> seen = new HashSet<>(); - seen.add(st); - Assertions.assertTrue(seen.contains(st), - "hashCode must agree with equals"); + try (Statement st = pooled.createStatement(); + Statement other = pooled.createStatement()) { + assertOpenStatementHandle(st, other, pooled, "the statement facade"); + assertOpenStatementHandle(statementBehindFacade(st), + statementBehindFacade(other), pooled, + "the decorator's own proxy"); } } } @Test void aClosedStatementHandleKeepsAnsweringWithItsOwnIdentity() throws Exception { - DataSource ds = decoratorPool(false); - Connection pooled = ds.getConnection(); - try { + DataSource ds = decoratorPool(); + try (Connection pooled = ds.getConnection()) { Statement st = pooled.createStatement(); - int hash = st.hashCode(); + // The decorator's proxy is reached while the handle is still + // open; it is the layer this test was written for. + Statement decoratorHandle = statementBehindFacade(st); + int facadeHash = st.hashCode(); + int decoratorHash = decoratorHandle.hashCode(); st.close(); - // Neither method declares a checked exception, so a throwing - // handler cannot reach the application as a SQLException - the - // proxy layer wraps it. Both answers must simply be values. - Assertions.assertDoesNotThrow(st::hashCode, - "hashCode on a closed handle must return, not throw"); - Assertions.assertEquals(hash, st.hashCode(), - "closing must not change the identity of a handle"); - Assertions.assertTrue(st.equals(st), - "closing must not break reflexivity of a handle"); - Assertions.assertFalse(st.equals(pooled), - "a closed handle is still equal to nothing but itself"); - try { - st.hashCode(); - } catch (UndeclaredThrowableException e) { - Assertions.fail("hashCode on a closed handle must not surface as " - + "UndeclaredThrowableException: " + e.getCause()); - } - } finally { - pooled.close(); + assertClosedStatementHandle(st, facadeHash, pooled, "the statement facade"); + assertClosedStatementHandle(decoratorHandle, decoratorHash, pooled, + "the decorator's proxy"); } } @Test void aResultSetHandleIsAnHonestHandleOpenAndClosed() throws Exception { - DataSource ds = decoratorPool(false); + DataSource ds = decoratorPool(); try (Connection pooled = ds.getConnection()) { - try (Statement st = pooled.createStatement()) { - ResultSet rs = st.executeQuery("SELECT 1"); - Assertions.assertTrue(rs.next()); + Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1"); + Assertions.assertTrue(rs.next()); + Statement decoratorSt = statementBehindFacade(st); + ResultSet decoratorRs = resultSetBehindFacade(rs); - Assertions.assertTrue(rs.equals(rs), - "a result set handle must be equal to itself"); - Assertions.assertFalse(rs.equals(null), - "a result set handle is not equal to null"); - Assertions.assertFalse(rs.equals(st), - "a result set handle is not equal to its statement"); - Assertions.assertFalse(rs.equals(new Object()), - "a result set handle is not equal to a foreign object"); - int hash = rs.hashCode(); - Assertions.assertEquals(hash, rs.hashCode(), - "hashCode must be a stable value of the handle itself"); + ResultSet facadeRs = rs; + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the decorator's own proxies"; + Statement stHandle = layer == 0 ? st : decoratorSt; + ResultSet rsHandle = layer == 0 ? facadeRs : decoratorRs; + + Assertions.assertTrue(rsHandle.equals(rsHandle), + name + ": a result set handle must be equal to itself"); + Assertions.assertFalse(rsHandle.equals(null), + name + ": a result set handle is not equal to null"); + Assertions.assertFalse(rsHandle.equals(stHandle), + name + ": a result set handle is not equal to its statement"); + Assertions.assertFalse(rsHandle.equals(new Object()), + name + ": a result set handle is not equal to a foreign object"); + int hash = rsHandle.hashCode(); + Assertions.assertEquals(hash, rsHandle.hashCode(), + name + ": hashCode must be a stable value of the handle itself"); // the handle a result set reports must answer to the - // statement handle the application holds - Assertions.assertSame(st, rs.getStatement()); - Assertions.assertTrue(rs.getStatement().equals(st), - "the reported statement must be equal to the held handle"); + // statement handle of the same layer + Assertions.assertSame(stHandle, rsHandle.getStatement(), + name + ": the statement behind the result set must be " + + "the handle of the same layer"); + Assertions.assertTrue(rsHandle.getStatement().equals(stHandle), + name + ": the reported statement must be equal to the held handle"); // toString is answered by the proxy, not forwarded to the // driver's cursor - Assertions.assertTrue(rs.toString().contains("ResultSetProxy"), - "the handle must describe itself, was: " + rs); - - rs.close(); - st.close(); - - Assertions.assertTrue(rs.equals(rs), - "a closed result set handle must still be equal to itself"); - Assertions.assertEquals(hash, rs.hashCode(), - "closing must not change the identity of a handle"); - Assertions.assertDoesNotThrow(() -> rs.equals(rs), - "equals on a closed handle must return, not throw"); + Assertions.assertTrue(rsHandle.toString().contains("ResultSetProxy"), + name + ": the handle must describe itself, was: " + rsHandle); + } + + int facadeRsHash = facadeRs.hashCode(); + int decoratorRsHash = decoratorRs.hashCode(); + // Close through the application's handles; both layers see it. + facadeRs.close(); + st.close(); + + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the decorator's proxy"; + ResultSet rsHandle = layer == 0 ? facadeRs : decoratorRs; + int hash = layer == 0 ? facadeRsHash : decoratorRsHash; + + Assertions.assertTrue(rsHandle.equals(rsHandle), + name + ": a closed result set handle must still be equal to itself"); + Assertions.assertEquals(hash, rsHandle.hashCode(), + name + ": closing must not change the identity of a handle"); + Assertions.assertDoesNotThrow(() -> rsHandle.equals(rsHandle), + name + ": equals on a closed handle must return, not throw"); } } } @@ -162,81 +215,108 @@ class TestStatementDecoratorIdentity extends JdbcPoolBaseTest { @Test void aReusedCachedStatementKeepsTheIdentityContract() throws Exception { DataSource ds = newDataSource(); - ds.getPoolProperties().setUseStatementFacade(false); ds.getPoolProperties().setJdbcInterceptors( StatementCache.class.getName() + "(prepared=true,callable=false,max=10)"); ds.createPool(); try (Connection pooled = ds.getConnection()) { PreparedStatement first = pooled.prepareStatement("SELECT 1"); - int firstHash = first.hashCode(); + Statement decoratorFirst = statementBehindFacade(first); + int decoratorFirstHash = decoratorFirst.hashCode(); first.close(); // recycled into the cache PreparedStatement reused = pooled.prepareStatement("SELECT 1"); + Statement decoratorReused = statementBehindFacade(reused); + // The facade hands out a fresh handle of its own for every + // borrow, and the cache re-parks the statement under a fresh + // proxy of its own; the reused statement must arrive with new + // identity at both layers. Assertions.assertNotSame(first, reused, "the cache hands the reused statement out under a fresh handle"); + Assertions.assertNotSame(decoratorFirst, decoratorReused, + "and the proxy below the facade is fresh as well"); Assertions.assertTrue(reused.equals(reused), - "a reused handle must be equal to itself"); + "a reused handle must be equal to itself at the facade layer"); + Assertions.assertTrue(decoratorReused.equals(decoratorReused), + "a reused handle must be equal to itself at the cache layer"); Assertions.assertFalse(reused.equals(first), "the disposed handle of the previous use stays unequal"); Assertions.assertFalse(first.equals(reused)); - Assertions.assertNotEquals(firstHash, reused.hashCode(), - "two handles must not share an identity hash"); - Assertions.assertEquals(reused.hashCode(), reused.hashCode(), + Assertions.assertFalse(decoratorReused.equals(decoratorFirst), + "and the disposed proxy of the previous use stays unequal " + + "below the facade"); + Assertions.assertFalse(decoratorFirst.equals(decoratorReused)); + Assertions.assertNotEquals(decoratorFirstHash, decoratorReused.hashCode(), + "two proxies must not share an identity hash"); + Assertions.assertEquals(decoratorReused.hashCode(), decoratorReused.hashCode(), "hashCode must be stable on a reused handle"); try (ResultSet rs = reused.executeQuery()) { Assertions.assertTrue(rs.next()); } - int reusedHash = reused.hashCode(); + int decoratorReusedHash = decoratorReused.hashCode(); reused.close(); - Assertions.assertEquals(reusedHash, reused.hashCode(), + Assertions.assertEquals(decoratorReusedHash, decoratorReused.hashCode(), "re-closing must not change the identity of a handle"); - Assertions.assertTrue(reused.equals(reused), + Assertions.assertTrue(decoratorReused.equals(decoratorReused), "a handle recycled a second time must still be equal to itself"); } } /* - * The facade absorbs the Object methods from outside the decorator, so - * the configuration switch must not change any answer the application - * gets. + * The facade answers the Object methods from outside the decorator, and + * since it is always there the application can never be routed around + * it - but the decorator's proxies stay live underneath, and the answers + * both layers give must obey the same contract. */ @Test - void bothConfigurationsAnswerTheSameWay() throws Exception { - for (boolean facade : new boolean[] { false, true }) { - String config = facade ? "with" : "without"; - DataSource ds = decoratorPool(facade); - try (Connection pooled = ds.getConnection()) { - Statement st = pooled.createStatement(); - ResultSet rs = st.executeQuery("SELECT 1"); - Assertions.assertTrue(rs.next(), config + " the facade"); - - Assertions.assertTrue(st.equals(st), - config + " the facade a statement handle is equal to itself"); - Assertions.assertFalse(st.equals(null), - config + " the facade equals(null) is false"); - Assertions.assertFalse(st.equals(rs), - config + " the facade a statement is not its result set"); - Assertions.assertTrue(rs.equals(rs), - config + " the facade a result set handle is equal to itself"); - Assertions.assertFalse(rs.equals(st), - config + " the facade a result set is not its statement"); - Assertions.assertTrue(rs.getStatement().equals(st), - config + " the facade the reported statement answers to the held handle"); - - int stHash = st.hashCode(); - int rsHash = rs.hashCode(); - rs.close(); - st.close(); - - Assertions.assertTrue(st.equals(st), - config + " the facade a closed statement stays equal to itself"); - Assertions.assertTrue(rs.equals(rs), - config + " the facade a closed result set stays equal to itself"); - Assertions.assertEquals(stHash, st.hashCode(), - config + " the facade the statement hash survives the close"); - Assertions.assertEquals(rsHash, rs.hashCode(), - config + " the facade the result set hash survives the close"); + void facadeHandleAndDecoratorHandleObeyTheSameContract() throws Exception { + DataSource ds = decoratorPool(); + try (Connection pooled = ds.getConnection()) { + Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1"); + Assertions.assertTrue(rs.next()); + Statement[] statements = { st, statementBehindFacade(st) }; + ResultSet[] resultSets = { rs, resultSetBehindFacade(rs) }; + + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the decorator's own proxies"; + Statement s = statements[layer]; + ResultSet r = resultSets[layer]; + + Assertions.assertTrue(s.equals(s), + name + ": a statement handle is equal to itself"); + Assertions.assertFalse(s.equals(null), + name + ": equals(null) is false"); + Assertions.assertFalse(s.equals(r), + name + ": a statement is not its result set"); + Assertions.assertTrue(r.equals(r), + name + ": a result set handle is equal to itself"); + Assertions.assertFalse(r.equals(s), + name + ": a result set is not its statement"); + Assertions.assertTrue(r.getStatement().equals(s), + name + ": the reported statement answers to the held handle"); + } + + int[] stHashes = { statements[0].hashCode(), statements[1].hashCode() }; + int[] rsHashes = { resultSets[0].hashCode(), resultSets[1].hashCode() }; + // Close through the application's handles; both layers then face + // the same closed state. + resultSets[0].close(); + statements[0].close(); + + for (int layer = 0; layer < 2; layer++) { + String name = layer == 0 ? "the statement facade" + : "the decorator's own proxies"; + + Assertions.assertTrue(statements[layer].equals(statements[layer]), + name + ": a closed statement stays equal to itself"); + Assertions.assertTrue(resultSets[layer].equals(resultSets[layer]), + name + ": a closed result set stays equal to itself"); + Assertions.assertEquals(stHashes[layer], statements[layer].hashCode(), + name + ": the statement hash survives the close"); + Assertions.assertEquals(rsHashes[layer], resultSets[layer].hashCode(), + name + ": the result set hash survives the close"); } } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java index fcf016bf93..ff7873601e 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java @@ -160,10 +160,6 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { @Test void theTrackingWrapperStaysHonestAboutTheStatementItWraps() throws Exception { DataSource ds = finalizerPool(); - // Without the statement facade the tracker's wrapper is the first - // proxy in front of the driver's own statement object, so it is the - // handle the application compares, closes and reuses. - ds.getPoolProperties().setUseStatementFacade(false); ds.createPool(); try (Connection pooled = ds.getConnection()) { @@ -171,12 +167,25 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { Assertions.assertNotNull(finalizer); PreparedStatement ps = pooled.prepareStatement("SELECT 1"); - Assertions.assertTrue(ps.equals(ps), "the handle must be equal to itself"); - Assertions.assertFalse(ps.equals(pooled), "an unrelated object must not compare equal"); + // The application handle is the statement facade's proxy, which + // is always installed. The tracking wrapper this interceptor + // puts in front of the driver's statement sits right beneath it, + // and that wrapper is what forgets the tracked entry at close - + // so its own contract is what the battery below pins down. + PreparedStatement wrapper = + (PreparedStatement) proxiedObject(ps); + Assertions.assertNotSame(ps, wrapper, + "the tracking wrapper must still sit between the facade and the driver"); + + Assertions.assertTrue(wrapper.equals(wrapper), "the handle must be equal to itself"); + Assertions.assertFalse(wrapper.equals(pooled), "an unrelated object must not compare equal"); + Statement driver = driverStatement(ps); + Assertions.assertTrue(wrapper.equals(driver), + "the wrapper must be equal to the statement it wraps"); java.util.Set<Statement> seen = new java.util.HashSet<>(); - seen.add(ps); - Assertions.assertTrue(seen.contains(ps), "hashCode must agree with equals"); - try (ResultSet rs = ps.executeQuery()) { + seen.add(wrapper); + Assertions.assertTrue(seen.contains(wrapper), "hashCode must agree with equals"); + try (ResultSet rs = wrapper.executeQuery()) { Assertions.assertTrue(rs.next()); } @@ -187,11 +196,11 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { Assertions.assertEquals(2, tracked(finalizer), "the callable statement must be tracked as well"); - ps.close(); - ps.close(); // JDBC demands that a repeated close is a no-op + wrapper.close(); + wrapper.close(); // JDBC demands that a repeated close is a no-op Assertions.assertEquals(1, tracked(finalizer), "the close must have been counted once"); - Assertions.assertThrows(SQLException.class, ps::getMoreResults, + Assertions.assertThrows(SQLException.class, wrapper::getMoreResults, "use after close must still be reported, not swallowed"); call.close(); Assertions.assertEquals(0, tracked(finalizer)); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index c8ba7b709a..887bdeaa63 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1297,6 +1297,16 @@ now runs before the return validation that would otherwise override it. (remm) </fix> + <fix> + Always wrap pooled statements and their result sets in the statement + facade, so a statement handle kept past the end of its borrow can no + longer execute against the session the next borrower holds, and a + statement reports the pooled connection it was created from rather + than the driver's. The <code>useStatementFacade</code> property that + could switch the facade off is deprecated and no longer has any + effect, but it is still accepted so existing configurations keep + loading. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
