This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 25904c956301d499d00e65f73b69c0f9397e0b5b Author: opencode <[email protected]> AuthorDate: Tue Nov 3 16:42:07 2026 +0100 Bring back every cleaner the dead shared timer took with it Replacing a shared timer that had already cancelled itself armed only the pool whose registration noticed the death. The other pools kept their entries in the registry, kept holding their tasks as the ones that served them, and were served by nothing: a timer task that has been scheduled once is spent - a task whose timer died is still marked as scheduled, and every timer refuses it from then on - so a new timer revives no task by itself. Nothing else could bring them back either. A task that is never scheduled again never runs to notice its own state, and the configuration check that looks the sweeper over asks only whether a task is registered and built on the interval the configuration wants, both of which remained true. Idle validation, eviction down to the min-idle floor and the abandoned check therefore stopped for every pool but the one that happened to arm a cleaner afterwards - the same silent, JVM-wide stoppage that the replacement was written to end, only quieter, since the pools concerned still reported themselves as swept. Rebuilding the registry is also not a thing the refusal can be trusted to guide. Timer.schedule answers IllegalStateException both for a timer that has cancelled itself and for a task that has been scheduled once before, and only the first of those a new timer cures. Reading either refusal as a dead timer makes a loop of it: a fresh timer built per turn, refusing the same spent task each time, the registry's monitor held throughout - and that monitor is one every pool's configuration write, close and cleaner registration waits behind. The replacement is now attempted once, on a timer built for the attempt. Acceptance there is the diagnosis, which the second cause cannot fake: a task the new timer takes was refused by the old timer's death, and the refusal that survives a brand new timer is the task's own, reported as the refusal it has always been with the registry left as the call found it - the timer it found may well be alive, and unbuilding it would be the same harm dressed as a repair. Where the death is confirmed, a one-shot on the new timer asks each pool that still holds a spent task to arm a fresh one, taking one pool's monitor at a time from a thread that holds none: pool threads reach the registry from behind their own monitors, so rebuilding the registry inside it would have crossed the two orders. A pool whose cleaner was meanwhile armed by a configuration write, or stopped by a close or by a sweeper switched off, is left as it is, and what the replacement found is counted in the log - a maintenance stoppage that heals itself should still say so. The pins: a pool whose sweep the dead timer took is seen sweeping again on the replacement, with the spent tasks gone from the registry and one entry per pool, and a task started a second time is refused shortly rather than retried, with the timer that was serving still the one serving. One assertion in the earlier test was also racing: it read the pool size at the moment the sweep was seen, while the size settles only when the retired session's physical close has run - a hundred microseconds later, and a connection left between the queues is what the assertion means to catch. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 167 ++++++++++++++++++--- .../tomcat/jdbc/pool/util/LocalStrings.properties | 3 + .../tomcat/jdbc/pool/TestSharedCleanerTimer.java | 146 +++++++++++++++++- 3 files changed, 286 insertions(+), 30 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 19ac544293..a4b336f39b 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 @@ -24,10 +24,12 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.SQLException; +import java.util.ArrayList; import java.util.Collections; import java.util.ConcurrentModificationException; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Set; import java.util.Timer; import java.util.TimerTask; @@ -986,6 +988,28 @@ public class ConnectionPool { } } + /** + * Arms a replacement for this pool's maintenance task when the task the pool still holds is the one that lost the + * shared timer, and does nothing when it is not. A pool whose cleaner has meanwhile been armed by a configuration + * write is served by that task, and one that has been stopped - by a close, or by a configuration that no longer + * asks for a sweeper - is served by none on purpose; reviving the spent task would either duplicate the + * maintenance work for the pool's lifetime or arm a pool that has given its connections back. + * + * @param stranded the task that was scheduled on the shared timer that died + */ + synchronized void rearmPoolCleaner(PoolCleaner stranded) { + if (poolCleaner != stranded) { + return; + } + // The spent task goes first whatever the configuration now asks for: its + // registry entry would otherwise stand for a task that can never run + // again, and no close would hand it back. What is armed after it is what + // the configuration asks for now, so a pool that has closed in the + // meantime, or has had its sweeper switched off, is left as it is. + terminatePoolCleaner(); + initializePoolCleaner(getPoolProperties()); + } + /** * Brings the pool cleaner in line with a configuration: what is running now is * compared against what the configuration asks for, including the interval the @@ -2780,33 +2804,128 @@ public class ConnectionPool { private static synchronized void registerCleaner(PoolCleaner cleaner) { unregisterCleaner(cleaner); cleaners.add(cleaner); - // A timer cancels itself when a task throws an uncaught throwable, - // and scheduling onto such a dead timer fails - which would make - // every pool that needs a cleaner afterwards fail the same way for - // the rest of the JVM's life. PoolCleaner.run no longer lets - // anything escape; this is the safety net for a timer that is - // already dead when it arrives: replace it rather than refuse. - while (true) { - if (poolCleanTimer == null) { - ClassLoader loader = Thread.currentThread().getContextClassLoader(); + if (poolCleanTimer == null) { + poolCleanTimer = newPoolTimer(); + } + try { + poolCleanTimer.schedule(cleaner, cleaner.sleepTime, cleaner.sleepTime); + return; + } catch (IllegalStateException x) { + // Two different failures answer with this exception and they call + // for opposite handling. A timer cancels itself for good the + // moment a task lets a throwable escape it, and every pool this + // class loader holds shares the one timer, so a timer that died + // refuses every pool that needs a cleaner afterwards for the rest + // of the JVM's life - unless it is replaced. The other failure is + // the task itself, which has been scheduled once already and no + // timer will ever accept again. The replacement is attempted on a + // timer built for the attempt, which is a question the second + // cause cannot fake: a task the new timer takes was refused by + // the old timer's death. Attempted once, because keyed on the + // refusal alone the second cause is incurable and the loop spins, + // handing out a fresh timer per turn and scheduling nothing on + // any of them. + Timer replacement = newPoolTimer(); + try { + replacement.schedule(cleaner, cleaner.sleepTime, cleaner.sleepTime); + } catch (IllegalStateException y) { + // The task is the spent one, so nothing is rebuilt. The timer + // this call found may well be alive and serving the tasks + // registered on it; the one just discarded was never asked to + // hold anything, and the registry goes back as this call found + // it. The refusal itself goes back to the caller, which is + // what starting a spent task has always answered with. + cleaners.remove(cleaner); + replacement.cancel(); + log.error(MessageFactory.get("connectionPool.cleanerSchedulingFailed"), y); + throw y; + } + // The diagnosis is the acceptance: every task the registry holds + // besides this one was scheduled on the timer that died. A spent + // task can never be scheduled again, so replacing the timer + // revives nothing by itself - each pool has to build a new task, + // and only that pool's monitor can arm it. Pool threads reach + // this method's monitor from behind their own, so arming them + // from here would cross the two orders; the new timer's thread + // holds no pool monitor and takes them one at a time. + List<PoolCleaner> stranded = new ArrayList<>(cleaners); + stranded.remove(cleaner); + poolCleanTimer = replacement; + if (!stranded.isEmpty()) { + log.error(MessageFactory.get("connectionPool.cleanerTimerReplaced", + Integer.valueOf(stranded.size())), x); try { - Thread.currentThread().setContextClassLoader(ConnectionPool.class.getClassLoader()); - // Create the timer thread in a PrivilegedAction so that a - // reference to the web application class loader is not created - // via Thread.inheritedAccessControlContext - poolCleanTimer = new Timer( - "Tomcat JDBC Pool Cleaner[" + System.identityHashCode(ConnectionPool.class.getClassLoader()) + - ":" + System.currentTimeMillis() + "]", - true); - } finally { - Thread.currentThread().setContextClassLoader(loader); + replacement.schedule(new CleanerReviver(stranded), 0); + } catch (RuntimeException z) { + // The recovery's own step is the one failing here. The pool + // that arrived has its task on the new timer and owes its + // configuration write no failure it did not cause, so this + // is reported rather than thrown back at it; the pools left + // stranded are what the message counts. + log.error(MessageFactory.get("connectionPool.cleanerRearmFailed"), z); } } - try { - poolCleanTimer.schedule(cleaner, cleaner.sleepTime, cleaner.sleepTime); - return; - } catch (IllegalStateException x) { - poolCleanTimer = null; + } + } + + /** + * Builds the timer that carries the maintenance of every pool this class + * loader holds. Its thread is created with the pool's own class loader as + * the context one, so that a reference to a web application class loader is + * not created via Thread.inheritedAccessControlContext. + * + * @return a timer with no tasks scheduled on it + */ + private static Timer newPoolTimer() { + ClassLoader loader = Thread.currentThread().getContextClassLoader(); + try { + Thread.currentThread().setContextClassLoader(ConnectionPool.class.getClassLoader()); + return new Timer( + "Tomcat JDBC Pool Cleaner[" + System.identityHashCode(ConnectionPool.class.getClassLoader()) + + ":" + System.currentTimeMillis() + "]", + true); + } finally { + Thread.currentThread().setContextClassLoader(loader); + } + } + + /** + * Arms a fresh maintenance task for each pool whose task was scheduled on + * the shared timer that died. The revival cannot reuse those tasks - one + * that has been scheduled once is spent - so it asks the pools themselves + * through {@link ConnectionPool#rearmPoolCleaner(PoolCleaner)}, which takes + * effect only for a pool still holding the spent task. + */ + private static final class CleanerReviver extends TimerTask { + private final List<PoolCleaner> stranded; + + CleanerReviver(List<PoolCleaner> stranded) { + this.stranded = stranded; + } + + @Override + public void run() { + for (PoolCleaner cleaner : stranded) { + try { + ConnectionPool pool = cleaner.pool.get(); + if (pool == null) { + // Nothing holds the pool any more. Its task will never + // run again, so it is the one entry in the registry that + // no cleaner will ever stop and no close will ever hand + // back - drop it here, as the task's own run method does + // when it outlives the pool. + unregisterCleaner(cleaner); + continue; + } + pool.rearmPoolCleaner(cleaner); + } catch (Throwable x) { + // The module's fatal policy runs first, as everywhere else. + // This is a timer task: a throwable that escaped it would + // take the replacement timer down the way the last one went, + // and the pools further down this list with it. + ExceptionUtils.handleThrowable(x); + log.error(MessageFactory.get("connectionPool.cleanerRearmFailed"), x); + } } } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 047bcb2315..b7d49bb1d3 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -37,6 +37,9 @@ classLoaderUtil.classLoadersNone=Unable to load class [{0}]: no class loaders su connectionPool.alternateUsernameIgnored=alternateUsernameAllowed is set to false so the provided user name [{0}] and associated password have been ignored for this borrow connectionPool.checkAbandonedFailed=checkAbandoned failed, it will be retried. connectionPool.checkIdleFailed=checkIdle failed, it will be retried. +connectionPool.cleanerRearmFailed=A pool maintenance task could not be armed again after the shared pool cleaner timer was replaced. That pool''s idle checks and abandoned connection check do not run until its sweeper is armed again. +connectionPool.cleanerSchedulingFailed=The pool maintenance task could not be scheduled on the pool cleaner timer and will not run. A timer task can be scheduled once only, so this task has been started before; create a new task rather than starting this one again. +connectionPool.cleanerTimerReplaced=The shared pool cleaner timer had stopped, so the maintenance tasks scheduled on it no longer run. A replacement timer has been created for the pool that needed one, and the {0} task or tasks the dead timer held are being armed again on it. connectionPool.connectionAbandoned=Connection has been abandoned {0}:{1} connectionPool.connectionSuspect=Connection has been marked suspect, possibly abandoned {0}[{1} ms.]:{2} connectionPool.evicterIntervalTooLow=Database connection pool evicter thread interval is set to lower than 1 second. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSharedCleanerTimer.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSharedCleanerTimer.java index a70990cb42..893bb5bea9 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSharedCleanerTimer.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSharedCleanerTimer.java @@ -17,8 +17,13 @@ package org.apache.tomcat.jdbc.pool; import java.sql.Connection; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; import java.util.Timer; +import java.util.TimerTask; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -33,12 +38,16 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; * abandoned-check for every pool in the JVM, later pool starts included: * scheduling onto the dead timer fails with {@code IllegalStateException} * and the pool-side field still holds it as if it were live. These tests - * pin the three parts of that contract: an ordinary {@code Error} inside + * pin the parts of that contract: an ordinary {@code Error} inside * the swept work is contained and the shared timer goes on serving, a - * timer that arrives dead is replaced rather than obeyed, and - so the - * containment cannot silently grow into swallowing the unrecoverable - - * the fatal conditions ExceptionUtils keeps fatal still propagate, from - * a borrow and from the task alike. + * timer that arrives dead is replaced rather than obeyed and the pools + * whose tasks it took with it are armed again on the replacement - a + * timer task that has been scheduled once is spent, so a new timer + * revives nothing by itself - while a task that no timer will ever take + * again is refused rather than retried, and - so the containment cannot + * silently grow into swallowing the unrecoverable - the fatal conditions + * ExceptionUtils keeps fatal still propagate, from a borrow and from the + * task alike. */ class TestSharedCleanerTimer extends JdbcPoolBaseTest { @@ -95,8 +104,12 @@ class TestSharedCleanerTimer extends JdbcPoolBaseTest { awaitUntil(() -> validator.idleCalls.get() >= 1); // the swept connection was read as a failed check and taken out of - // service - not stranded mid-sweep + // service - not stranded mid-sweep. The size settles only once the + // retired session's physical close has run, a step after the sweep + // that decided it, so it is awaited rather than raced; a connection + // left between the two queues never reaches zero. Assertions.assertEquals(0, first.getPool().getIdle()); + awaitUntil(() -> first.getPool().getSize() == 0); Assertions.assertEquals(0, first.getPool().getSize()); // the timer outlived the sweep: a second pool starts on the very @@ -141,6 +154,127 @@ class TestSharedCleanerTimer extends JdbcPoolBaseTest { "the sweep must have run into the error the guard is tested against"); } + /** Vouches for every session it is asked about, counting the idle checks. */ + public static class IdleCheckCounter implements Validator { + private final AtomicInteger idleCalls = new AtomicInteger(); + + @Override + public boolean validate(Connection connection, int validateAction) { + if (validateAction == PooledConnection.VALIDATE_IDLE) { + idleCalls.incrementAndGet(); + } + return true; + } + } + + @Test + void aDeadSharedTimerTakesDownNoOtherPoolSweeper() throws Exception { + IdleCheckCounter counter = new IdleCheckCounter(); + DataSource swept = newDataSource(); + swept.getPoolProperties().setMinIdle(1); + swept.getPoolProperties().setTestWhileIdle(true); + swept.getPoolProperties().setTimeBetweenEvictionRunsMillis(200); + swept.getPoolProperties().setValidationInterval(0); + swept.getPoolProperties().setValidator(counter); + swept.createPool(); + + DataSource other = newDataSource(); + other.getPoolProperties().setTimeBetweenEvictionRunsMillis(500); + other.createPool(); + + Set<TimerTask> scheduled = new HashSet<>(ConnectionPool.getPoolCleaners()); + Assertions.assertEquals(2, scheduled.size(), + "both pools must be served by the one shared timer"); + + Connection borrowed = swept.getConnection(); + borrowed.close(); + // the idle sweep has really reached the parked session + awaitUntil(() -> counter.idleCalls.get() >= 1); + + // The state a task that died on a fatal error leaves behind for + // every pool: the shared timer cancelled for good, and the tasks that + // were scheduled on it spent - a timer task that has been scheduled + // once can never be scheduled again, so a replacement timer revives + // nothing by itself. + Timer dead = ConnectionPool.getPoolTimer(); + Assertions.assertNotNull(dead); + dead.cancel(); + + DataSource next = newDataSource(); + next.getPoolProperties().setTimeBetweenEvictionRunsMillis(500); + next.createPool(); + Assertions.assertNotSame(dead, ConnectionPool.getPoolTimer(), + "the refusal must have been read as a dead timer and answered with a fresh one"); + + // The pool whose task the dead timer took has to be maintaining again, + // not merely registered: its sweep reaches the parked session a second + // time on the replacement timer. + awaitUntil(() -> counter.idleCalls.get() >= 2); + Assertions.assertTrue(Collections.disjoint(scheduled, ConnectionPool.getPoolCleaners()), + "the tasks that lost the dead timer must have left the registry"); + Assertions.assertEquals(3, ConnectionPool.getPoolCleaners().size(), + "every pool must be represented by the task that now serves it"); + + next.close(); + other.close(); + swept.close(); + } + + @Test + void aCleanerTaskStartedTwiceIsRefusedRatherThanLoopedOn() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(500); + ds.createPool(); + Timer serving = ConnectionPool.getPoolTimer(); + Assertions.assertNotNull(serving); + + ConnectionPool.PoolCleaner twice = + new ConnectionPool.PoolCleaner(ds.getPool(), 200); + twice.start(); + AtomicReference<Throwable> thrown = new AtomicReference<>(); + try { + // The second start is the arrival a new timer cannot cure: the task + // itself has been scheduled and given up, and no timer will ever + // take it again. Keyed on the refusal alone, this is a loop that + // builds a fresh timer per turn, never schedules anything, and + // holds the registry's monitor for the rest of the JVM's life - so + // the refusal has to come back as the refusal it is, and shortly. + Thread attempt = new Thread(() -> { + try { + twice.start(); + } catch (Throwable x) { + thrown.set(x); + } + }, "second-start-of-one-cleaner"); + attempt.setDaemon(true); + attempt.start(); + attempt.join(10000); + Assertions.assertFalse(attempt.isAlive(), + "a refusal no timer can cure must be reported, not retried"); + Assertions.assertInstanceOf(IllegalStateException.class, thrown.get(), + "starting a spent task keeps answering with the refusal it always did"); + } finally { + twice.stopRunning(); + } + + Assertions.assertFalse(ConnectionPool.getPoolCleaners().contains(twice), + "the refused task must not be left claiming a registration"); + Assertions.assertSame(serving, ConnectionPool.getPoolTimer(), + "the timer that was serving must be the one still serving"); + + // and it is serving: a pool that needs a cleaner joins it unchanged + DataSource next = newDataSource(); + next.getPoolProperties().setTimeBetweenEvictionRunsMillis(500); + next.createPool(); + Assertions.assertSame(serving, ConnectionPool.getPoolTimer()); + try (Connection c = next.getConnection()) { + Assertions.assertNotNull(c); + } + + next.close(); + ds.close(); + } + @Test void aDeadSharedTimerIsReplacedWhenANewPoolNeedsOne() throws Exception { DataSource first = newDataSource(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
