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 13337d940c45b4539931288e3d2694b48b32c91e Author: opencode <[email protected]> AuthorDate: Mon Oct 26 11:32:51 2026 +0100 Close tracked statements properly and forget closed ones StatementFinalizer removed its tracking entries only in bulk at connection return and tracked statements through weak references without a queue. A borrow that closed every statement it created was still charged one entry per created statement for the whole borrow cycle, and exactly the statements the interceptor exists for - those an application forgot about - were the ones whose referent got collected: their finalisation was then skipped silently and their database-side state reached the next borrower. Entries are now strong references in a list guarded by a monitor the pool cleaner thread shares, and createStatement hands the application a thin close-aware proxy that drops the entry the moment a close passes through it, keeping the tracked list bounded by the statements that are genuinely still open; leaked statements are closed at the return regardless of the garbage collector. A failed close keeps the entry so the return retries, a statement whose isClosed() check fails is closed anyway, and trace mode retains creation stacks only for statements that actually leak. The added proxy layer costs roughly 35ns per statement call and 80ns per statement creation, below what the other documented interceptors of the module already charge and under a percent of a real database round trip; applications running with useStatementFacade=false now receive one thin proxy around tracked statements where they previously received the driver object, which close tracking requires. Regression tests cover close-forgets-entry, leaked-statement survival through garbage collection and closure at the return, and wrapper identity semantics with the facade disabled. --- .../jdbc/pool/interceptor/StatementFinalizer.java | 199 +++++++++++++++++---- .../pool/interceptor/TestStatementFinalizer.java | 198 ++++++++++++++++++++ webapps/docs/changelog.xml | 17 ++ 3 files changed, 379 insertions(+), 35 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java index 84843ff395..7d4adf292c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java @@ -16,9 +16,13 @@ */ package org.apache.tomcat.jdbc.pool.interceptor; -import java.lang.ref.WeakReference; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.sql.CallableStatement; +import java.sql.PreparedStatement; import java.sql.Statement; +import java.util.ArrayList; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -32,15 +36,35 @@ import org.apache.tomcat.jdbc.pool.PooledConnection; /** * Keeps track of statements associated with a connection and invokes close upon {@link java.sql.Connection#close()} * Useful for applications that don't close the associated statements after being done with a connection. + * <p> + * A statement is tracked with a strong reference until it is closed: an + * application that forgot about a statement is exactly the case the + * interceptor exists for, so finalisation must not depend on that statement + * still being reachable when the connection is returned. Statements that the + * application does close are forgotten by the returned proxy at the moment + * <code>close()</code> completes, keeping the tracked list bounded by the + * statements that are genuinely still open. (An interceptor placed outside + * this one may swallow the statement's <code>close()</code>, e.g. a statement + * cache recycling instead of closing; such entries are simply reclaimed at + * the connection return, as they were before.) * */ public class StatementFinalizer extends AbstractCreateStatementInterceptor { private static final Log log = LogFactory.getLog(StatementFinalizer.class); /** - * List of statements created on the associated connection. + * Statements created on the associated connection that have not been + * closed yet. Every access must be guarded by {@link #statementsLock}. */ - protected List<StatementEntry> statements = new LinkedList<>(); + protected final List<StatementEntry> statements = new LinkedList<>(); + + /** + * Monitor guarding {@link #statements}. The pool's cleaner thread may + * clear the list (through {@link #reset}) while a borrower is still + * creating or closing statements on an abandoned connection, so + * subclasses that access the list must hold this lock as well. + */ + protected final Object statementsLock = new Object(); /** * Default constructor. @@ -48,41 +72,60 @@ public class StatementFinalizer extends AbstractCreateStatementInterceptor { public StatementFinalizer() { } - private boolean logCreationStack = false; + private volatile boolean logCreationStack = false; @Override public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { + if (!(statement instanceof Statement)) { + return statement; + } + Statement tracked = (Statement) statement; + String name = method.getName(); + int constructorIndex; + Class<?> statementInterface; + if (compare(CREATE_STATEMENT, name)) { + constructorIndex = CREATE_STATEMENT_IDX; + statementInterface = Statement.class; + } else if (compare(PREPARE_STATEMENT, name)) { + constructorIndex = PREPARE_STATEMENT_IDX; + statementInterface = PreparedStatement.class; + } else if (compare(PREPARE_CALL, name)) { + constructorIndex = PREPARE_CALL_IDX; + statementInterface = CallableStatement.class; + } else { + // not one of the statement creation methods; nothing to track + return tracked; + } + StatementEntry entry = new StatementEntry(tracked); + synchronized (statementsLock) { + statements.add(entry); + } try { - if (statement instanceof Statement) { - statements.add(new StatementEntry((Statement)statement)); - } - }catch (ClassCastException x) { - //ignore this one + return getConstructor(constructorIndex, statementInterface) + .newInstance(new Object[] { new CloseAwareProxy(entry) }); + } catch (Exception x) { + // The statement is still tracked and will be closed when the + // connection is returned, but closing it early will no longer + // forget it before then. + log.warn("Unable to create statement tracking proxy.", x); + return tracked; } - return statement; } - @SuppressWarnings("null") // st is not null when used @Override public void closeInvoked() { - while (!statements.isEmpty()) { - StatementEntry ws = statements.remove(0); - Statement st = ws.getStatement(); - boolean shallClose = false; - try { - shallClose = st!=null && (!st.isClosed()); - if (shallClose) { - st.close(); - } - } catch (Exception ignore) { - if (log.isDebugEnabled()) { - log.debug("Unable to closed statement upon connection close.",ignore); - } - } finally { - if (logCreationStack && shallClose) { - log.warn("Statement created, but was not closed at:", ws.getAllocationStack()); - } + List<StatementEntry> unclosed; + synchronized (statementsLock) { + if (statements.isEmpty()) { + return; } + unclosed = new ArrayList<>(statements); + statements.clear(); + } + // Closing a statement can block on the database, so it happens + // outside the lock that guards the list. + for (StatementEntry entry : unclosed) { + entry.close(); } } @@ -98,34 +141,40 @@ public class StatementFinalizer extends AbstractCreateStatementInterceptor { @Override public void reset(ConnectionPool parent, PooledConnection con) { - statements.clear(); + synchronized (statementsLock) { + statements.clear(); + } super.reset(parent, con); } /** - * Entry that tracks a statement and its allocation stack. + * Entry that tracks a statement and its allocation stack. The statement + * is held strongly: it must stay closeable until the application or the + * connection return disposes of it. */ protected class StatementEntry { - private WeakReference<Statement> statement; - private Throwable allocationStack; + private final Statement statement; + private final Throwable allocationStack; /** * Creates a new StatementEntry. * @param statement the statement to track */ public StatementEntry(Statement statement) { - this.statement = new WeakReference<>(statement); + this.statement = statement; if (logCreationStack) { this.allocationStack = new Throwable(); + } else { + this.allocationStack = null; } } /** * Returns the tracked statement. - * @return the statement, or null if it has been garbage collected + * @return the statement */ public Statement getStatement() { - return statement.get(); + return statement; } /** @@ -135,7 +184,87 @@ public class StatementFinalizer extends AbstractCreateStatementInterceptor { public Throwable getAllocationStack() { return allocationStack; } + + /** + * Closes the tracked statement, reporting failure the same way the + * connection return does. + */ + public void close() { + boolean shallClose = false; + try { + shallClose = !statement.isClosed(); + } catch (Exception ignore) { + // A driver that cannot say whether the statement is closed + // must not get to skip the close attempt - the statement + // may still hold server-side state. + shallClose = true; + } + if (shallClose) { + try { + statement.close(); + } catch (Exception ignore) { + if (log.isDebugEnabled()) { + log.debug("Unable to close statement upon connection close.", ignore); + } + } + } + if (logCreationStack && shallClose) { + log.warn("Statement created, but was not closed at:", allocationStack); + } + } } + /** + * Thin proxy around a tracked statement. Every call is delegated + * unchanged; only <code>close()</code> is reacted to, by forgetting the + * tracked entry so that a statement the application closed properly + * neither lingers in the list nor is touched again when the connection + * is returned. Identity follows the statement underneath: the proxy is + * equal to itself and to its delegate, and hashes like its delegate, + * though the delegate's own equality - typically identity - will not + * recognise the proxy in the other direction. + */ + protected class CloseAwareProxy implements InvocationHandler { + private final StatementEntry entry; + + /** + * Creates a handler for the given tracked statement. + * @param entry the list entry tracking the statement + */ + public CloseAwareProxy(StatementEntry entry) { + this.entry = entry; + } + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (compare(EQUALS_VAL, method)) { + // Identity of the handle the application holds, else the + // statement underneath - a proxy that answered equals by + // delegating would not be equal to itself. + Object other = args[0]; + return Boolean.valueOf(other == proxy || entry.getStatement().equals(other)); + } + if (compare(HASHCODE_VAL, method)) { + return Integer.valueOf(entry.getStatement().hashCode()); + } + Object result; + try { + result = method.invoke(entry.getStatement(), args); + } catch (InvocationTargetException ite) { + // The close failed in the statement - keep the entry so the + // connection return tries again. + Throwable cause = ite.getTargetException(); + if (cause != null) { + throw cause; + } + throw ite; + } + if (compare(CLOSE_VAL, method)) { + synchronized (statementsLock) { + statements.remove(entry); + } + } + return result; + } + } } 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 new file mode 100644 index 0000000000..7bd8dad63f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java @@ -0,0 +1,198 @@ +/* + * 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.interceptor; + +import java.lang.reflect.Field; +import java.sql.CallableStatement; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * What {@code StatementFinalizer} owes the application from the other + * side of the coin: statements the application <em>did</em> close must be + * forgotten the moment they are closed (the tracked list is bounded by + * genuinely open statements, not by everything the borrow ever created), + * and statements it <em>did not</em> close must survive garbage collection + * so they can still be closed when the connection is returned. + */ +class TestStatementFinalizer extends JdbcPoolBaseTest { + + private DataSource finalizerPool() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(StatementFinalizer.class.getName()); + return ds; + } + + /* + * The tracker's own list, read reflectively so the assertions are about + * the tracker's state rather than about any particular field or entry + * type it happens to use internally. + */ + @SuppressWarnings("unchecked") + private static List<Object> trackedEntries(StatementFinalizer finalizer) throws Exception { + Field field = StatementFinalizer.class.getDeclaredField("statements"); + field.setAccessible(true); + return (List<Object>) field.get(finalizer); + } + + private static int tracked(StatementFinalizer finalizer) throws Exception { + return trackedEntries(finalizer).size(); + } + + /* The statement a tracked entry remembers; null if it let go of it. */ + private static Statement trackedStatement(Object entry) throws Exception { + try { + return (Statement) entry.getClass().getMethod("getStatement").invoke(entry); + } catch (java.lang.reflect.InvocationTargetException e) { + throw new AssertionError(e.getCause()); + } + } + + @Test + void closedStatementsAreForgottenTheMomentTheyAreClosed() throws Exception { + DataSource ds = finalizerPool(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + StatementFinalizer finalizer = interceptorOf(pooled, StatementFinalizer.class); + assertNotNull(finalizer, "the interceptor must sit in the chain of the borrowed connection"); + + for (int i = 0; i < 250; i++) { + try (Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + rs.next(); + } + try (PreparedStatement ps = pooled.prepareStatement("SELECT ?")) { + ps.setInt(1, i); + try (ResultSet rs = ps.executeQuery()) { + rs.next(); + } + } + } + + assertEquals(0, tracked(finalizer), + "a borrow that closed every statement must not be charged for them at connection return"); + + Statement openOne = pooled.createStatement(); + PreparedStatement openTwo = pooled.prepareStatement("SELECT 1"); + try (ResultSet rs = openOne.executeQuery("SELECT 1")) { + rs.next(); + } + try (ResultSet rs = openTwo.executeQuery()) { + rs.next(); + } + assertEquals(2, tracked(finalizer), + "exactly the statements that are still open must be tracked"); + + openOne.close(); + assertEquals(1, tracked(finalizer), "closing must remove the entry, not wait for the return"); + openTwo.close(); + assertEquals(0, tracked(finalizer), "closing must remove the entry, not wait for the return"); + } + } + + @Test + void aStatementTheApplicationLeakedIsStillTrackedAfterGarbageCollection() throws Exception { + DataSource ds = finalizerPool(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + StatementFinalizer finalizer = interceptorOf(pooled, StatementFinalizer.class); + assertNotNull(finalizer); + + Statement leaked = pooled.createStatement(); + try (ResultSet rs = leaked.executeQuery("SELECT 1")) { + rs.next(); + } + + leaked = null; + // Best-effort collection: the contract this test pins down is the + // close-at-return asserted below, which holds whatever the garbage + // collector does. The collection attempt is what makes the null + // check above bite a weak-reference implementation. + for (int i = 0; i < 3; i++) { + System.gc(); + } + + List<Object> entries = trackedEntries(finalizer); + assertEquals(1, entries.size(), "the leaked statement must still be tracked at the return"); + // A handle taken only after the collection: before the fix the + // referent could be gone by then, and the interceptor would have + // nothing left to close when the connection returns. + Statement stillTracked = trackedStatement(entries.get(0)); + assertNotNull(stillTracked, + "finalisation must not depend on the leaked statement still being reachable"); + + pooled.close(); + assertTrue(stillTracked.isClosed(), + "the leaked statement must be closed when its connection goes back"); + } + + @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()) { + StatementFinalizer finalizer = interceptorOf(pooled, StatementFinalizer.class); + assertNotNull(finalizer); + + PreparedStatement ps = pooled.prepareStatement("SELECT 1"); + assertTrue(ps.equals(ps), "the handle must be equal to itself"); + assertFalse(ps.equals(pooled), "an unrelated object must not compare equal"); + java.util.Set<Statement> seen = new java.util.HashSet<>(); + seen.add(ps); + assertTrue(seen.contains(ps), "hashCode must agree with equals"); + try (ResultSet rs = ps.executeQuery()) { + assertTrue(rs.next()); + } + + CallableStatement call = pooled.prepareCall("CALL NOW()"); + try (ResultSet rs = call.executeQuery()) { + assertTrue(rs.next()); + } + 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 + assertEquals(1, tracked(finalizer), "the close must have been counted once"); + + assertThrows(SQLException.class, ps::getMoreResults, + "use after close must still be reported, not swallowed"); + call.close(); + assertEquals(0, tracked(finalizer)); + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 17ca9e38c6..2481acb610 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1062,6 +1062,23 @@ need no restore, keep the previous behaviour of leaving the transaction untouched. (remm) </fix> + <fix> + Correct two defects in the <code>StatementFinalizer</code> + interceptor. Tracked statements were only removed in bulk when the + connection was returned, so a borrow that closed every statement it + created still accumulated one tracking entry per statement created + for the whole borrow cycle; and the tracking relied on weak + references without a queue, so exactly the statements the + interceptor exists for - those an application forgot about - could + be garbage collected before being closed, leaking their + database-side state to the next borrower. Statements are now + tracked with a strong reference in a list guarded against the + pool's cleaner thread, and the statement proxy handed to the + application removes its tracking entry as soon as the application + closes the statement, while a leaked statement is reliably closed + when the connection is returned regardless of the garbage + collector. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
