This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 87007cef2a6e1346056d7fcafbbdd4cd2d606e08 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 22:04:03 2026 +0100 Release what the abandoned-timer handles wrap once they are closed ResetAbandonedTimer's statement and result-set proxies kept their wrapped layer as final fields that outlived the close: a handle the application retained after its statement or result set was closed kept the statement layer, the physical result set and - through the inner class's link to the interceptor - the connection graph reachable for as long as the handle was held, longer than the statement facade's own handles retain, which release their delegate when a close passes through them. Apply both rules the neighbouring proxies already follow. Release: null the delegate - and the result-set proxy's statement link - once a close has been forwarded, so a retained closed handle stops pinning the objects below it. And to make that safe, snapshot: a close can come from a thread other than the caller's - the statement finalizer closes the statements it tracks from whichever thread closes the connection, and it can track these handles when it is listed above this interceptor - so the fields are now volatile and read exactly once per call, with the guards and the forward agreeing on the one value the call read, as committed for the facade in b207ff5fbc and for the query report proxy in 495b82258a. A call that finds the delegate gone is answered from the closed contract the facade and the query report proxy use - close is a no-op, isClosed answers true, anything else throws "Statement closed."/"ResultSet closed." - instead of Method.invoke's NullPointerException on a null receiver, and for result-set reads the refusal still lands inside the try block, so the attempted-operation-is-activity rule that resets the timer even for failed calls keeps working exactly as committed in 74b8c5717a. Through the always-on facade no answer an application can observe changes. --- .../jdbc/pool/interceptor/ResetAbandonedTimer.java | 132 +++++++++++--- .../TestResetAbandonedTimerHandleRetention.java | 190 +++++++++++++++++++++ 2 files changed, 301 insertions(+), 21 deletions(-) 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 5da165526c..3a2f326d54 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 @@ -23,6 +23,7 @@ import java.lang.reflect.Proxy; import java.sql.CallableStatement; import java.sql.PreparedStatement; import java.sql.ResultSet; +import java.sql.SQLException; import java.sql.Statement; import java.sql.Wrapper; @@ -42,10 +43,14 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; * itself blocks on the database for longer than * {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setRemoveAbandonedTimeout(int)} 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. + * it can go on refreshing the abandoned timer of whoever holds that physical connection next. The handles this + * interceptor hands out release what they wrap - the statement layer, the physical result set - once a close has been + * forwarded to it, and answer any later call from the closed contract, as the statement facade's own handles do; a + * handle the application keeps beyond its close therefore stops keeping the physical objects below it reachable. 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. */ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAbandonedTimerMBean { @@ -172,9 +177,17 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba */ private final class TimerStatementProxy implements InvocationHandler { /** - * The statement proxy created by {@code AbstractQueryReport}. + * The statement proxy created by {@code AbstractQueryReport}, nulled + * once a close has been forwarded so that a handle the application + * keeps beyond its close stops pinning the statement layer beneath it. + * A close can come from a thread other than the one invoking on the + * handle - the pool's statement finalizer closes the statements it + * tracks from whichever thread closes the connection - so the field is + * read by more threads than the one that nulls it and, as in the + * facade's and the query report proxy, must be volatile to reach them + * at all. */ - private final Object delegate; + private volatile Object delegate; TimerStatementProxy(Object delegate) { this.delegate = delegate; @@ -197,9 +210,35 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba if (compare(HASHCODE_VAL, method)) { return Integer.valueOf(this.hashCode()); } + // Read the delegate once for this call: the close that nulls it + // can come from another thread, and the guards and the forward + // must agree on the one value this call read. A call that finds + // the delegate gone is answered from the closed contract below - + // the same answers the query report proxy beneath gives for its + // own closed state - rather than by invoking on a null receiver: + // that is a NullPointerException, where the closed contract owes + // the application a SQLException. The window this cannot close is + // the race itself: a call that read the delegate while it was + // still live goes to it, an outcome indistinguishable from having + // simply outrun the close. + final Object target = delegate; + final boolean close = compare(CLOSE_VAL, method); + // Allow close to be called multiple times, from either thread + if (close && target == null) { + return null; + } + if (compare(ISCLOSED_VAL, method) && target == null) { + return Boolean.TRUE; + } + if (target == null) { + throw new SQLException("Statement closed."); + } Object result; try { - result = method.invoke(delegate, args); + // Execute the call on the delegate this call read as live: + // the snapshot keeps a concurrent close from replacing the + // receiver with null between the guards and the call + result = method.invoke(target, args); } catch (InvocationTargetException x) { Throwable cause = x.getCause(); if (cause == null) { @@ -207,6 +246,11 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba } throw cause; } + // Perform close cleanup: the handle kept beyond this close now + // holds nothing that can reach the statement beneath it + if (close) { + delegate = null; + } // Wrap every result set the call hands back, whatever the method // that returned it was called, on the same rule the statement // facade uses: a decision made on the method name only sees the @@ -256,16 +300,23 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba */ private final class TimerResultSetProxy implements InvocationHandler { /** - * The result set being read. + * The result set being read, nulled once a close has been forwarded so + * that a handle the application keeps beyond its close stops pinning + * the physical result set. Read by other threads than the one that + * nulls it - a close can come from a foreign thread, as in the + * statement proxy - so, as in the facade and the query report proxy, + * volatile. */ - private final Object delegate; + private volatile Object delegate; /** * The statement handle in use by this interceptor, returned by {@link java.sql.ResultSet#getStatement()}. It * keeps the timer reset like the handle the application holds, but it is not necessarily that same outermost * handle: the statement facade substitutes its own handle from outside this interceptor, and any interceptor - * configured around this one may too. + * configured around this one may too. Nulled along with {@link #delegate} once a close has been forwarded, so + * that the handle of a closed result set stops pinning the statement layer either; read in the same snapshot as + * the delegate, so it is volatile for the same reason. */ - private final Object statement; + private volatile Object statement; TimerResultSetProxy(Object delegate, Object statement) { this.delegate = delegate; @@ -289,20 +340,59 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba if (compare(HASHCODE_VAL, method)) { return Integer.valueOf(this.hashCode()); } - if (compare(GET_STATEMENT, method)) { - return statement; - } try { - return method.invoke(delegate, args); - } catch (InvocationTargetException x) { - Throwable cause = x.getCause(); - if (cause == null) { - throw x; + // Read both fields once for this call: the close that nulls + // them can come from another thread, and the guards and the + // forward must agree on the one value this call read. The + // close nulls the delegate before the statement, so reading + // the statement first keeps the snapshot coherent: a call that + // read the delegate as live cannot have missed the statement. + final Object stmt = statement; + final Object target = delegate; + final boolean close = compare(CLOSE_VAL, method); + // Allow close to be called multiple times, from either thread + if (close && target == null) { + return null; } - throw cause; + if (compare(ISCLOSED_VAL, method) && target == null) { + return Boolean.TRUE; + } + if (target == null) { + // Answered from the closed contract, as the facade's own + // result set handle does, rather than by invoking on a + // null receiver: that is a NullPointerException, where the + // closed contract owes the application a SQLException + throw new SQLException("ResultSet closed."); + } + if (compare(GET_STATEMENT, method)) { + return stmt; + } + Object result; + try { + // Invoke on the delegate this call read as live: the + // snapshot keeps a concurrent close from replacing the + // receiver with null between the guards and the call + result = method.invoke(target, args); + } catch (InvocationTargetException x) { + Throwable cause = x.getCause(); + if (cause == null) { + throw x; + } + throw cause; + } + // Perform close cleanup: the handle kept beyond this close now + // holds nothing that can reach the cursor or the statement + if (close) { + delegate = null; + statement = null; + } + return result; } finally { // Reading the result set is activity on the connection, - // even when the read failed + // even when the read failed. The guards above sit inside + // this block, so a read refused on a handle whose delegate + // has been released counts as activity too, like every + // other attempted operation on this connection. resetTimer(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerHandleRetention.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerHandleRetention.java new file mode 100644 index 0000000000..c51d018f9d --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimerHandleRetention.java @@ -0,0 +1,190 @@ +/* + * 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.lang.reflect.Proxy; +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; +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.PooledConnection; + +/** + * How long the statement and result set handles the + * {@code ResetAbandonedTimer} interceptor hands out beneath the facade + * keep the objects they wrap. + * + * <p>Once a close has been forwarded, the interceptor's own handle must let + * go of the layer beneath it - the query-report proxy over the statement, and + * the driver's cursor and statement layer for a result set - so that a handle + * an application keeps beyond its close stops pinning the physical objects, + * the same rule the statement facade's own handles obey (see + * {@code TestStatementHandleLifetime} for the facade's lifetime, and + * {@code TestStatementFacadeCrossThreadClose} for the close race the volatile + * snapshot here answers). What a released handle still owes the caller is the + * closed contract - an honest {@code isClosed()}, an idempotent + * {@code close()} and a refusal on everything else - and a refused read still + * counts as activity on the connection, the contract the failing-operation + * keep-alive established.</p> + */ +class TestResetAbandonedTimerHandleRetention extends JdbcPoolBaseTest { + + private DataSource timerPool() throws SQLException { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.ResetAbandonedTimer"); + ds.createPool(); + return ds; + } + + /* + * A named field of any pool object the tests need to see into - the + * interceptor's own fields (its pooled connection) and the fields of an + * invocation handler, which the caller passes through + * {@link Proxy#getInvocationHandler}. The base class' proxiedObject only + * reads the delegate field of a handler; this reaches the rest. + */ + private static Object fieldOf(Object target, String name) + throws ReflectiveOperationException { + Field field = target.getClass().getDeclaredField(name); + field.setAccessible(true); + return field.get(target); + } + + private static Object handlerField(Object handle, String name) + throws ReflectiveOperationException { + return fieldOf(Proxy.getInvocationHandler(handle), name); + } + + @Test + void aClosedStatementHandleReleasesTheLayerBeneathIt() throws Exception { + DataSource ds = timerPool(); + try (Connection con = ds.getConnection()) { + Statement st = con.createStatement(); + Object timerHandle = proxiedObject(st); + Assertions.assertTrue(Proxy.getInvocationHandler(timerHandle).getClass().getName() + .contains("TimerStatementProxy"), + "precondition: beneath the facade sits this interceptor's handle, found " + + Proxy.getInvocationHandler(timerHandle).getClass().getName()); + Assertions.assertNotNull(proxiedObject(timerHandle), + "the open handle must hold the query-report layer beneath it"); + + st.close(); + + Assertions.assertNull(proxiedObject(timerHandle), + "a closed handle must release the layer beneath it"); + + // The closed contract on this interceptor's own handle: honest + // isClosed, idempotent close, and a refusal that says so - the + // same answers the handle gave once its own delegate beneath had + // closed, short-circuited here now that this delegate is gone. + Statement timerStatement = (Statement) timerHandle; + Assertions.assertTrue(timerStatement.isClosed(), + "the released handle must report itself closed"); + timerStatement.close(); + timerStatement.close(); + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> timerStatement.executeQuery("SELECT 1"), + "a released handle must refuse to execute"); + Assertions.assertEquals("Statement closed.", thrown.getMessage(), + "the refusal must say what happened"); + + // The application's own handle behaves as before, unchanged by + // what the layer beneath it now does with its reference. + Assertions.assertTrue(st.isClosed(), + "the application's handle reports itself closed"); + st.close(); + Assertions.assertThrows(SQLException.class, () -> st.executeQuery("SELECT 1"), + "the application's handle still refuses after close"); + } + } + + @Test + void aClosedResultSetHandleReleasesTheCursorAndTheStatement() throws Exception { + DataSource ds = timerPool(); + try (Connection con = ds.getConnection(); + Statement st = con.createStatement()) { + ResultSet rs = st.executeQuery("SELECT X FROM SYSTEM_RANGE(1,3)"); + Object timerHandle = proxiedObject(rs); + Assertions.assertTrue(Proxy.getInvocationHandler(timerHandle).getClass().getName() + .contains("TimerResultSetProxy"), + "precondition: beneath the facade sits this interceptor's handle, found " + + Proxy.getInvocationHandler(timerHandle).getClass().getName()); + Assertions.assertNotNull(proxiedObject(timerHandle), + "the open handle must hold the driver cursor"); + Assertions.assertNotNull(handlerField(timerHandle, "statement"), + "the open handle must hold the statement layer"); + + rs.close(); + + Assertions.assertNull(proxiedObject(timerHandle), + "a closed result set handle must release the driver cursor"); + Assertions.assertNull(handlerField(timerHandle, "statement"), + "a closed result set handle must release the statement layer"); + + // The closed contract on the application's own handle, unchanged. + Assertions.assertTrue(rs.isClosed(), + "the application's result set handle reports itself closed"); + rs.close(); + Assertions.assertThrows(SQLException.class, rs::next, + "the application's result set handle still refuses reads after close"); + } + } + + /* + * The attempted-operation-is-activity contract, on the one call a + * released handle now refuses itself: a read that gets refused for + * having no delegate left is still an operation attempted on the + * connection, and must reset the abandoned timer just like every read + * that reached the driver - failing included (see + * TestResetAbandonedTimerStreaming.failedResultSetReadsKeepConnectionAlive). + * The refusal is invoked on the interceptor's own handle, captured + * before the close: through the facade the application's handle is + * refused one layer up, which is its own sealed-contract behavior. + */ + @Test + void aReadRefusedByAReleasedHandleIsStillActivity() throws Exception { + DataSource ds = timerPool(); + try (Connection con = ds.getConnection()) { + ResetAbandonedTimer timer = interceptorOf(con, ResetAbandonedTimer.class); + Assertions.assertNotNull(timer, "precondition: the interceptor is in the chain"); + try (Statement st = con.createStatement()) { + ResultSet rs = st.executeQuery("SELECT X FROM SYSTEM_RANGE(1,3)"); + ResultSet timerHandle = (ResultSet) proxiedObject(rs); + rs.close(); // through the facade, releasing this delegate too + + PooledConnection pcon = (PooledConnection) fieldOf(timer, "pcon"); + pcon.setTimestamp(1L); // as if this borrow had all but expired + + SQLException thrown = Assertions.assertThrows(SQLException.class, + timerHandle::next, + "the released handle must refuse the read itself"); + Assertions.assertEquals("ResultSet closed.", thrown.getMessage(), + "the refusal must be the closed contract, not a driver error"); + Assertions.assertTrue(pcon.getTimestamp() > 1L, + "the refused read must still have reset the abandoned timer"); + } + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
