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 89aa1d38fb0b740c0f872f7cd022c55127bc9568 Author: opencode <[email protected]> AuthorDate: Sun Oct 11 22:38:37 2026 +0200 Guard the time bucket counter maintenance futures against a racing destroy() The maintenanceFuture field of TimeBucketCounterBase was written by the MaintenanceMonitor task on the executor thread and read, cancelled and nulled by destroy() on a container thread without any happens-before edge between the two. Within that window the monitor could observe the stale, already cancelled future, treat it as a failed task, schedule a replacement periodic Maintenance task and publish it after destroy() had nulled the field. The replacement was then never cancelled: on a shared executor it kept running for the lifetime of the executor, retaining the destroyed counter and its bucket map. Introduce a destroyed flag and use the object monitor to guard it together with both future fields. destroy() sets the flag, clears the map and cancels the futures as one critical section, and the monitor returns immediately when the flag is set, so a task can no longer be scheduled after the counter has been destroyed. The scheduling in the constructor also happens under the monitor now, making the guarded invariant hold for every access; the object escapes to the executor at that point. destroy() becomes documented as thread safe and idempotent. --- .../catalina/util/TimeBucketCounterBase.java | 59 ++++++++++++++-------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/java/org/apache/catalina/util/TimeBucketCounterBase.java b/java/org/apache/catalina/util/TimeBucketCounterBase.java index 7aec8de153..85c5d04738 100644 --- a/java/org/apache/catalina/util/TimeBucketCounterBase.java +++ b/java/org/apache/catalina/util/TimeBucketCounterBase.java @@ -43,9 +43,12 @@ public abstract class TimeBucketCounterBase { // Map to hold the buckets private final ConcurrentHashMap<String,AtomicInteger> map = new ConcurrentHashMap<>(); - // The future allowing control of the background processor. + // The futures controlling the background tasks, plus the destroyed flag. All three are guarded by this + // object's monitor, which is held by destroy() on a container thread and by MaintenanceMonitor on the + // executor thread. private ScheduledFuture<?> maintenanceFuture; private ScheduledFuture<?> monitorFuture; + private boolean destroyed; private final ScheduledExecutorService executorService; private final long sleeptime; private final int bucketDuration; @@ -73,7 +76,12 @@ public abstract class TimeBucketCounterBase { // Start our thread if (sleeptime > 0) { - monitorFuture = executorService.scheduleWithFixedDelay(new MaintenanceMonitor(), 0, 60, TimeUnit.SECONDS); + // Synchronize, since this is the one write not performed under the monitor by the maintenance or + // destroy() threads, and the object escapes to the executor while the constructor is still running + synchronized (this) { + monitorFuture = executorService.scheduleWithFixedDelay(new MaintenanceMonitor(), 0, 60, + TimeUnit.SECONDS); + } } } @@ -167,17 +175,21 @@ public abstract class TimeBucketCounterBase { /** - * Stops threads created by this object and cleans up resources. + * Stops threads created by this object and cleans up resources. This method is thread safe and idempotent. On + * return, no maintenance or monitor task remains scheduled. */ public void destroy() { - map.clear(); - if (monitorFuture != null) { - monitorFuture.cancel(true); - monitorFuture = null; - } - if (maintenanceFuture != null) { - maintenanceFuture.cancel(true); - maintenanceFuture = null; + synchronized (this) { + destroyed = true; + map.clear(); + if (monitorFuture != null) { + monitorFuture.cancel(true); + monitorFuture = null; + } + if (maintenanceFuture != null) { + maintenanceFuture.cancel(true); + maintenanceFuture = null; + } } } @@ -211,17 +223,24 @@ public abstract class TimeBucketCounterBase { private class MaintenanceMonitor implements Runnable { @Override public void run() { - if (sleeptime > 0 && (maintenanceFuture == null || maintenanceFuture.isDone())) { - if (maintenanceFuture != null && maintenanceFuture.isDone()) { - // There was an error executing the scheduled task, get it and log it - try { - maintenanceFuture.get(); - } catch (InterruptedException | ExecutionException e) { - log.error(sm.getString("timebucket.maintenance.error"), e); + synchronized (TimeBucketCounterBase.this) { + if (destroyed) { + // destroy() has run. Do not schedule any replacement task, or the new future would never be + // cancelled and the destroyed counter would be retained by the executor for its lifetime. + return; + } + if (sleeptime > 0 && (maintenanceFuture == null || maintenanceFuture.isDone())) { + if (maintenanceFuture != null && maintenanceFuture.isDone()) { + // There was an error executing the scheduled task, get it and log it + try { + maintenanceFuture.get(); + } catch (InterruptedException | ExecutionException e) { + log.error(sm.getString("timebucket.maintenance.error"), e); + } } + maintenanceFuture = executorService.scheduleWithFixedDelay(new Maintenance(), sleeptime, + sleeptime, TimeUnit.MILLISECONDS); } - maintenanceFuture = executorService.scheduleWithFixedDelay(new Maintenance(), sleeptime, sleeptime, - TimeUnit.MILLISECONDS); } } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
