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 725dddb2c732eee61b3fa2cd8d88cefe74e73bd0 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 e38406cd26..4565997419 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 @@ -99,7 +99,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 @@ -439,9 +439,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); @@ -618,21 +616,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]
