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 d40813b41a7a9cb8739df37a7c2024a9a69dc758 Author: opencode <[email protected]> AuthorDate: Sun Oct 25 22:41:03 2026 +0100 Replace the pool cleaner atomically so no task is orphaned initializePoolCleaner() and terminatePoolCleaner() acted on the poolCleaner field without any lock and the initializer overwrote a running cleaner. Concurrent sweeper-enable updates via JMX could leave the replaced task registered on the shared timer, duplicating the pool's maintenance work with no reference left to stop it and outliving close(). Synchronize both lifecycle methods, terminate a still running cleaner before starting the replacement, and let close() go through the same gate. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 20 +++++--- .../tomcat/jdbc/pool/TestDuplicatePoolCleaner.java | 53 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 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 53b45567e4..ea0a078f06 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 @@ -97,7 +97,7 @@ public class ConnectionPool { /** * The thread that is responsible for checking abandoned and idle threads */ - private volatile PoolCleaner poolCleaner; + private PoolCleaner poolCleaner; /** * Pool closed flag @@ -437,9 +437,7 @@ public class ConnectionPool { //prevent other threads from entering this.closed = true; //stop background thread - if (poolCleaner!=null) { - poolCleaner.stopRunning(); - } + terminatePoolCleaner(); /* release all idle connections */ BlockingQueue<PooledConnection> pool = (!idle.isEmpty())?idle:(force?busy:idle); @@ -616,21 +614,29 @@ public class ConnectionPool { /** * Starts the pool cleaner (evicter) thread if pool sweeper is enabled. + * A cleaner that is still running from a previous start is terminated + * first, so this call never leaves a replaced task registered on the + * shared timer without a reference to stop it. * * @param properties the pool configuration containing sweeper settings */ - public void initializePoolCleaner(PoolConfiguration properties) { + public synchronized void initializePoolCleaner(PoolConfiguration properties) { //if the evictor thread is supposed to run, start it now if (properties.isPoolSweeperEnabled()) { + // never overwrite a running cleaner: the replaced task would + // stay registered on the shared timer with no reference left to + // stop it, duplicating maintenance work for the pool's lifetime + terminatePoolCleaner(); poolCleaner = new PoolCleaner(this, properties.getTimeBetweenEvictionRunsMillis()); poolCleaner.start(); } //end if } /** - * Stops the pool cleaner (evicter) thread if it is currently running. + * Stops the pool cleaner (evicter) thread if it is currently running + * and forgets its reference. */ - public void terminatePoolCleaner() { + public synchronized void terminatePoolCleaner() { if (poolCleaner!= null) { poolCleaner.stopRunning(); poolCleaner = null; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDuplicatePoolCleaner.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDuplicatePoolCleaner.java new file mode 100644 index 0000000000..10e147639d --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDuplicatePoolCleaner.java @@ -0,0 +1,53 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A pool must never have more than one pool cleaner scheduled. Two + * overlapping sweeper-enable updates (the JMX attribute path runs + * check-then-act without any lock) otherwise overwrite the pool's cleaner + * reference while the replaced task stays registered on the shared timer, + * duplicating the maintenance work for as long as the pool is reachable, + * with no reference left to stop it. + */ +class TestDuplicatePoolCleaner extends JdbcPoolBaseTest { + + @Test + void closingThePoolUnregistersEveryCleanerItStarted() throws Exception { + int baseline = ConnectionPool.getPoolCleaners().size(); + DataSource ds = newDataSource(); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + // The pool's own initialization already started one cleaner; a + // second initializer run is what an overlapping sweeper-enable + // update performs (the sequential shape reproduces the outcome of + // the race deterministically). + pool.initializePoolCleaner(ds.getPoolProperties()); + + ds.close(); + + assertEquals(baseline, ConnectionPool.getPoolCleaners().size(), + "closing the pool must unregister every cleaner it started; " + + "a replaced cleaner must not stay scheduled"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
