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 0b9b7238a740256b090b1f0ab8d7272a6b72a807 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. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 22 +++- .../pool/TestForcedShutdownDoesNotAbandon.java | 126 +++++++++++++++++++++ 2 files changed, 142 insertions(+), 6 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 f82b4a9b26..6d5217aa09 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 @@ -572,13 +572,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]
