This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 56ef71a67e Review Fix BZ 69236 - reset JDBC interceptors in
declaration order
56ef71a67e is described below
commit 56ef71a67e7833fa8d9043ba46f44f9c7604d9f9
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Oct 23 20:53:32 2026 +0100
Review Fix BZ 69236 - reset JDBC interceptors in declaration order
Add a changelog entry
Fix a few leaks
Improve error reporting
Refactor the test
---
.../apache/tomcat/jdbc/pool/ConnectionPool.java | 28 +++-
.../interceptor/TestInterceptorResetOrder.java | 175 +++++++++++++++++++++
.../jdbc/test/InterceptorResetOrderTest.java | 106 -------------
webapps/docs/changelog.xml | 10 ++
4 files changed, 211 insertions(+), 108 deletions(-)
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 a0460cbf3c..b021fb7d40 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
@@ -314,6 +314,8 @@ public class ConnectionPool {
}catch(Exception x) {
SQLException sx = new SQLException("Unable to instantiate
interceptor chain.");
sx.initCause(x);
+ //nothing has been done to the connection, it can go back
to the pool
+ abandonSetup(con, sx, false);
throw sx;
}
}
@@ -321,15 +323,18 @@ public class ConnectionPool {
con.setHandler(handler);
}
//reset the chain in the order the interceptors are declared, for new
and cached handlers alike
+ JdbcInterceptor next = handler;
try {
- JdbcInterceptor next = handler;
while (next!=null) {
next.reset(this, con);
next = next.getNext();
}
}catch(Exception x) {
- SQLException sx = new SQLException("Unable to reset interceptor
chain.");
+ SQLException sx = new SQLException("Unable to reset interceptor ["
+
+ (next!=null?next.getClass().getName():"unknown") + "].");
sx.initCause(x);
+ //the state of the connection is unknown, discard it so its
partially reset chain is dropped
+ abandonSetup(con, sx, true);
throw sx;
}
// setup statement proxy
@@ -351,11 +356,30 @@ public class ConnectionPool {
}catch (Exception x) {
SQLException s = new SQLException();
s.initCause(x);
+ abandonSetup(con, s, false);
throw s;
}
}
+ /**
+ * Gives up a connection that was borrowed but could not be set up, so
that it does not stay borrowed forever.
+ * Any failure to do so is added to the exception that is about to be
thrown rather than hiding it.
+ * @param con the borrowed connection
+ * @param cause the exception that is about to be thrown to the caller
+ * @param discard {@code true} if the connection must be closed, {@code
false} if it can go back to the pool
+ */
+ private void abandonSetup(PooledConnection con, SQLException cause,
boolean discard) {
+ try {
+ if (discard) {
+ con.setDiscarded(true);
+ }
+ returnConnection(con);
+ }catch(Exception y) {
+ cause.addSuppressed(y);
+ }
+ }
+
/**
* Creates and caches a {@link java.lang.reflect.Constructor} used to
instantiate the proxy object.
* We cache this, since the creation of a constructor is fairly slow.
diff --git
a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorResetOrder.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorResetOrder.java
new file mode 100644
index 0000000000..41c439fad6
--- /dev/null
+++
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorResetOrder.java
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.jdbc.pool.interceptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import org.apache.tomcat.jdbc.JdbcPoolBaseTest;
+import org.apache.tomcat.jdbc.pool.ConnectionPool;
+import org.apache.tomcat.jdbc.pool.DataSource;
+import org.apache.tomcat.jdbc.pool.JdbcInterceptor;
+import org.apache.tomcat.jdbc.pool.PooledConnection;
+
+/**
+ * The interceptors of a connection must be reset in the order in which they
are declared, whether the connection is
+ * new or is taken from the pool, and a failure to reset them must not leave
the connection borrowed.
+ */
+class TestInterceptorResetOrder extends JdbcPoolBaseTest {
+
+ private static final List<String> RESETS =
Collections.synchronizedList(new ArrayList<>());
+
+ @BeforeEach
+ void clearResets() {
+ RESETS.clear();
+ Unconstructable.fail = false;
+ }
+
+ private DataSource pool(Class<?>... interceptors) {
+ DataSource ds = newDataSource();
+ ds.getPoolProperties().setMaxActive(1);
+ StringBuilder chain = new StringBuilder();
+ for (Class<?> interceptor : interceptors) {
+ if (chain.length() > 0) {
+ chain.append(';');
+ }
+ chain.append(interceptor.getName());
+ }
+ ds.getPoolProperties().setJdbcInterceptors(chain.toString());
+ return ds;
+ }
+
+ @Test
+ void resetOrderOnNewConnection() throws Exception {
+ DataSource ds = pool(First.class, Second.class);
+
+ ds.getConnection().close();
+
+ assertEquals(Arrays.asList("first", "second"), RESETS);
+ }
+
+ @Test
+ void resetOrderOnReusedConnection() throws Exception {
+ DataSource ds = pool(First.class, Second.class);
+ try (Connection first = ds.getConnection()) {
+ setSessionMarker(first, 1);
+ }
+ RESETS.clear();
+
+ try (Connection reused = ds.getConnection()) {
+ assertEquals(Integer.valueOf(1), sessionMarker(reused),
+ "the cached interceptor chain must be exercised on the
same physical session");
+ assertEquals(Arrays.asList("first", "second"), RESETS);
+ }
+ }
+
+ @Test
+ void resetFailureIsReportedAndTheConnectionDiscarded() throws Exception {
+ DataSource ds = pool(First.class, Failing.class, Second.class);
+
+ SQLException e = assertThrows(SQLException.class, () ->
ds.getConnection());
+
+ assertInstanceOf(IllegalStateException.class, e.getCause());
+ assertTrue(e.getMessage().contains(Failing.class.getName()),
e.getMessage());
+ // The interceptors after the one that failed are not reset
+ assertEquals(Arrays.asList("first"), RESETS);
+ // The connection that could not be set up must not stay borrowed ...
+ assertEquals(0, ds.getPool().getActive());
+ // ... and must have been closed rather than returned to the pool
+ assertEquals(0, ds.getPool().getIdle());
+ awaitSessions(0);
+ }
+
+ @Test
+ void instantiationFailureIsReportedAndTheConnectionReturned() throws
Exception {
+ DataSource ds = pool(Unconstructable.class);
+ // With a single connection a leaked borrow would make the second
attempt wait for the connection
+ ds.getPoolProperties().setMaxWait(500);
+ // The pool instantiates the interceptors when it starts so only fail
after that
+ ds.createPool();
+ Unconstructable.fail = true;
+
+ for (int i = 0; i < 2; i++) {
+ SQLException e = assertThrows(SQLException.class, () ->
ds.getConnection());
+
+ assertTrue(e.getMessage().contains("instantiate"), e.getMessage());
+ // Nothing was done to the connection so it goes back to the pool
rather than being closed
+ assertEquals(0, ds.getPool().getActive());
+ assertEquals(1, ds.getPool().getIdle());
+ }
+ awaitSessions(1);
+ }
+
+ public static class First extends JdbcInterceptor {
+
+ @Override
+ public void reset(ConnectionPool parent, PooledConnection con) {
+ // the pool also resets interceptors without a parent when a
connection is discarded
+ if (parent != null) {
+ RESETS.add("first");
+ }
+ }
+ }
+
+ public static class Second extends JdbcInterceptor {
+
+ @Override
+ public void reset(ConnectionPool parent, PooledConnection con) {
+ if (parent != null) {
+ RESETS.add("second");
+ }
+ }
+ }
+
+ public static class Unconstructable extends JdbcInterceptor {
+
+ static volatile boolean fail;
+
+ public Unconstructable() {
+ if (fail) {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void reset(ConnectionPool parent, PooledConnection con) {
+ // NO-OP
+ }
+ }
+
+ public static class Failing extends JdbcInterceptor {
+
+ @Override
+ public void reset(ConnectionPool parent, PooledConnection con) {
+ if (parent != null) {
+ throw new IllegalStateException();
+ }
+ }
+ }
+}
diff --git
a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/InterceptorResetOrderTest.java
b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/InterceptorResetOrderTest.java
deleted file mode 100644
index a4824702cf..0000000000
---
a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/InterceptorResetOrderTest.java
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.tomcat.jdbc.test;
-
-import java.sql.SQLException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
-
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-
-import org.apache.tomcat.jdbc.pool.ConnectionPool;
-import org.apache.tomcat.jdbc.pool.JdbcInterceptor;
-import org.apache.tomcat.jdbc.pool.PooledConnection;
-import org.apache.tomcat.jdbc.test.driver.Driver;
-
-/**
- * The reset order of the interceptors must follow the order in which they are
declared, whether the connection is new
- * or is taken from the pool.
- */
-public class InterceptorResetOrderTest extends DefaultTestCase {
-
- private static final List<String> RESETS =
Collections.synchronizedList(new ArrayList<>());
-
- @Before
- public void setUp() {
- RESETS.clear();
- datasource.setDriverClassName(Driver.class.getName());
- datasource.setUrl("jdbc:tomcat:test");
- datasource.setMaxActive(1);
- datasource.setInitialSize(0);
- datasource.setMinIdle(0);
- datasource.setJdbcInterceptors(First.class.getName() + ";" +
Second.class.getName());
- }
-
- @Test
- public void testResetOrderOnNewConnection() throws SQLException {
- datasource.getConnection().close();
- Assert.assertEquals(Arrays.asList("first", "second"), RESETS);
- }
-
- @Test
- public void testResetOrderOnReusedConnection() throws SQLException {
- datasource.getConnection().close();
- RESETS.clear();
- datasource.getConnection().close();
- Assert.assertEquals(Arrays.asList("first", "second"), RESETS);
- }
-
- @Test
- public void testResetFailureIsReportedAsSQLException() {
- datasource.setJdbcInterceptors(Failing.class.getName());
-
- SQLException e = Assert.assertThrows(SQLException.class, () ->
datasource.getConnection());
-
- Assert.assertTrue(e.getCause() instanceof IllegalStateException);
- }
-
- public static class Failing extends JdbcInterceptor {
-
- @Override
- public void reset(ConnectionPool parent, PooledConnection con) {
- if (parent != null) {
- throw new IllegalStateException();
- }
- }
- }
-
- public static class First extends JdbcInterceptor {
-
- @Override
- public void reset(ConnectionPool parent, PooledConnection con) {
- // the pool also resets interceptors without a parent when a
connection is discarded
- if (parent != null) {
- RESETS.add("first");
- }
- }
- }
-
- public static class Second extends JdbcInterceptor {
-
- @Override
- public void reset(ConnectionPool parent, PooledConnection con) {
- if (parent != null) {
- RESETS.add("second");
- }
- }
- }
-}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 73b924209b..0b8bb8f803 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -986,6 +986,16 @@
configuration. Pull request <pr>1084</pr> provided by Raphael
Vullriede.
(markt)
</fix>
+ <fix>
+ <bug>69236</bug>: Reset the interceptors of a new connection in the
+ order they are declared, as is already done for a connection taken from
+ the pool, rather than in reverse order. A failure to reset an
+ interceptor is now reported as an <code>SQLException</code> that names
+ the interceptor and the connection is discarded rather than left
+ borrowed. A connection is also no longer left borrowed if the
+ interceptors cannot be instantiated or the connection proxy cannot be
+ created. Pull request provided by Arturo Bernal. (markt)
+ </fix>
<fix>
<bug>70269</bug>: Correct a regression in the change that reset the
properties of statements cached by the <code>StatementCache</code>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
