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 0e36743b2fefaaddc0d0612808ad49d827fbd9d1 Author: opencode <[email protected]> AuthorDate: Thu Oct 29 18:40:26 2026 +0100 Do not count connections torn down by a forced pool close as abandoned The close(true) shutdown drain tore connections that were legitimately in use down through abandon(), the path that reports an application leak, so every deliberate forced shutdown inflated removeAbandonedCount by one per active connection, logged an abandoned-connection warning with the borrow trace under logAbandoned, and raised one CONNECTION ABANDONED JMX notification per borrower. Release the drained busy connections the way the same drain releases idle ones - disconnecting them and running the interceptors' disconnect events - and leave the abandonment accounting to checkAbandoned(), which reports the leaks it actually detects. The handles a shutdown takes away are disconnected and refuse use exactly as before; TestForcedShutdownDoesNotAbandon pins the count, the notification and the drain. --- modules/jdbc-pool/TESTING-PLAN.md | 17 ++- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 22 +++- .../pool/TestForcedShutdownDoesNotAbandon.java | 126 +++++++++++++++++++++ 3 files changed, 153 insertions(+), 12 deletions(-) diff --git a/modules/jdbc-pool/TESTING-PLAN.md b/modules/jdbc-pool/TESTING-PLAN.md index 369ef21e5d..575bf1edd4 100644 --- a/modules/jdbc-pool/TESTING-PLAN.md +++ b/modules/jdbc-pool/TESTING-PLAN.md @@ -292,12 +292,17 @@ the working list. closes the *physical* connection, so the pool discards it instead of pooling it; returning only the `XAConnection` keeps it (what `TestXaConnection` asserts). - * `ConnectionPool.close()` releases idle connections via - `release()` but checked-out ones via `abandon()`, and only the - former unregisters the per-connection - `...,connections=PooledConnection[n]` MBean — a pool closed - while connections are borrowed leaves those MBeans in the - platform server. + * `ConnectionPool.close(true)` used to tear down connections that were + still borrowed via `abandon()`, booking a forced shutdown as one + application leak per connection in use (inflated + `removeAbandonedCount`, a WARN with the borrow trace under + `logAbandoned`, one `CONNECTION ABANDONED` notification each). The + drain now releases busy connections the way it releases idle ones — + disconnecting them and unregistering their + `...,connections=PooledConnection[n]` MBeans, as a release always + did through `abandon()` — and the abandonment bookkeeping stays + `checkAbandoned()`'s alone; `TestForcedShutdownDoesNotAbandon` + pins it. * Found while harvesting, same status (unintended-looking, so not pinned): * `ProxyConnection`'s own `equals`/`hashCode`, reachable with the 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 f244103019..9d3f8d9cf0 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 @@ -570,13 +570,23 @@ public class ConnectionPool { PooledConnection con = pool.poll(1000, TimeUnit.MILLISECONDS); // Close it and retrieve the next one, if one is available while (con != null) { - // Close the connection + // Close the connection. A forced shutdown tears a busy + // connection away from a borrower that is legitimately + // using it, so the drain releases it the way it releases + // an idle one: disconnecting the session and running the + // interceptors' disconnect events. It does not abandon it. + // The abandonment counter, the WARN log and the JMX + // abandon notification record an application that leaked + // a connection - checkAbandoned()'s bookkeeping - and a + // shutdown the operator ordered is no such leak, so + // recording it there inflates the monitoring counter and + // fires one bogus leak notification per connection in + // use. The borrower's handle is left equally unusable + // either way: an abandon disconnected it through this + // same release, and a later close on the handle finds the + // pool closed and the connection already released. try { - if (pool == idle) { - release(con); - } else { - abandon(con); - } + release(con); } catch (Throwable x) { // One failing connection must not leave the // remaining idle and busy connections undisconnected diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestForcedShutdownDoesNotAbandon.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestForcedShutdownDoesNotAbandon.java new file mode 100644 index 0000000000..760cc83945 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestForcedShutdownDoesNotAbandon.java @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +import javax.management.Notification; +import javax.management.NotificationListener; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The drain that {@code close(true)} runs over the busy queue tears down + * connections that are legitimately in use, on the operator's order. That + * is not an application leaking a connection, which is the one thing the + * pool's abandonment bookkeeping - the {@code removeAbandonedCount} + * counter, the {@code logAbandoned} WARN and the {@code NOTIFY_ABANDON} + * JMX notification - reports, and it belongs to + * {@link ConnectionPool#checkAbandoned()} alone. The forced shutdown must + * therefore drop every one of the leak accounting, the warning and the + * notification, while still fully disconnecting and draining the pool: + * the borrowed handles end up unusable, no physical session survives the + * close, and the borrowers' late returns stay quiet no-ops. + */ +class TestForcedShutdownDoesNotAbandon extends JdbcPoolBaseTest { + + @Test + void forcedShutdownReleasesBusyConnectionsWithoutRecordingThemAsAbandoned() + throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration properties = ds.getPoolProperties(); + properties.setJmxEnabled(true); + properties.setName("ForcedShutdown-" + System.nanoTime()); + // the WARN the abandon path logs is gated on this; the shutdown + // drain must stay silent under it as well as without it + properties.setLogAbandoned(true); + properties.setInitialSize(0); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmxPool = pool.getJmxPool(); + Assertions.assertNotNull(jmxPool, "the fixture must have created the pool MBean"); + + List<Notification> abandoned = new CopyOnWriteArrayList<>(); + NotificationListener abandonedListener = (notification, handback) -> { + if (org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_ABANDON + .equals(notification.getType())) { + abandoned.add(notification); + } + }; + jmxPool.addListener(abandonedListener); + + // two connections legitimately in use when the shutdown is ordered + Connection first = ds.getConnection(); + Connection second = ds.getConnection(); + Assertions.assertEquals(2, poolSessions(), + "the fixture must own both physical sessions before the close"); + + ds.close(true); + + Assertions.assertEquals(0, pool.getRemoveAbandonedCount(), + "a connection that was in use during a forced shutdown was not" + + " abandoned by the application and must not inflate the" + + " removed-abandoned counter"); + + // Delivery runs on the pool's single notifier thread in the order the + // notifications were raised, so a sentinel queued behind the close is + // only reached once everything the shutdown itself notified of (had it + // notified at all) has been delivered. + List<Notification> flushed = new CopyOnWriteArrayList<>(); + NotificationListener sentinelListener = (notification, handback) -> { + if (org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY + .equals(notification.getType())) { + flushed.add(notification); + } + }; + jmxPool.addListener(sentinelListener); + Assertions.assertTrue(jmxPool.notify( + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY, "flush sentinel"), + "the fixture must be able to queue its sentinel notification"); + awaitUntil(() -> !flushed.isEmpty()); + Assertions.assertTrue(abandoned.isEmpty(), + "a forced shutdown must not report a leak to JMX, got " + + abandoned.size() + " abandon notifications"); + + // What the shutdown must still do, unchanged: the handle it took away + // is disconnected and refuses use + Assertions.assertTrue(first.isClosed(), + "the forced shutdown must disconnect the connection it takes away"); + Assertions.assertThrows(SQLException.class, first::createStatement, + "a handle whose session the shutdown tore down must refuse use"); + + // and the pool drained completely: the in-use sessions left with it + awaitSessions(0); + Assertions.assertEquals(0, pool.getSize(), + "the forced shutdown must release every connection it drained"); + + // the late return of a dead handle is a quiet no-op that neither + // fails nor counts anything + first.close(); + second.close(); + Assertions.assertEquals(0, pool.getSize(), + "returning an already released handle must not move the pool size"); + Assertions.assertEquals(0, pool.getRemoveAbandonedCount(), + "a late return to a closed pool must not be counted as abandoned"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
