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 483bcc3d7d96daaf4bbb881e0b8cc25251f0a68b Author: opencode <[email protected]> AuthorDate: Thu Nov 5 21:13:41 2026 +0100 Serialize configuration validation with cleaner reconciliation The data source setters synchronize on the data source, while the pool MBean setters hold no monitor. Each called checkPoolConfiguration and then the synchronized reconcilePoolCleaner, so two writers could run their validation against different intermediate states and publish an inconsistent configuration: maxIdle above maxActive, minIdle or initialSize past the active limit, or a cleaner task on an interval the correction had already replaced. Add ConnectionPool.applyConfigurationChange(), synchronized on the pool, which runs the validation and the cleaner reconciliation as one step, and route both management surfaces through it. Pool creation keeps its existing publication sequence. Add a regression test that parks both writers in the check and verifies their checks cannot overlap. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 22 +++++ .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 11 ++- .../tomcat/jdbc/pool/jmx/ConnectionPool.java | 10 +- .../jdbc/pool/TestPoolSizeSetterValidation.java | 108 +++++++++++++++++++++ 4 files changed, 142 insertions(+), 9 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 1d0658e995..6b6ef4e991 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 @@ -1101,6 +1101,10 @@ public class ConnectionPool { /** * Validates and corrects pool configuration values. Ensures that maxActive, initialSize, minIdle, maxIdle, and * related values are consistent. + * <p> + * A running pool's writers must not call this alone: the correction can change the eviction interval the cleaner + * runs on, and a change made outside this object's monitor can interleave with another surface's write between + * the correction and the cleaner reconciliation. See {@link #applyConfigurationChange(PoolConfiguration)}. * * @param properties the pool configuration to validate and correct */ @@ -1147,6 +1151,24 @@ public class ConnectionPool { } } + /** + * Validates a running pool's configuration and brings the pool cleaner in line with the result as one step under + * this pool's monitor. + * <p> + * The two halves belong together: the validation corrects values the caller did not write - the eviction interval + * among them - and the cleaner's period is fixed when its task is built, so validating without then looking the + * cleaner over leaves a task on the interval the correction replaced. Running them under one monitor is what makes + * the pair atomic against the other management surface: the data source holds its own monitor while it writes the + * shared configuration, and the pool's MBean holds none, so this object's monitor is the only thing both writers + * are certain to serialise on. + * + * @param properties the pool configuration to validate and reconcile the cleaner with + */ + public synchronized void applyConfigurationChange(PoolConfiguration properties) { + checkPoolConfiguration(properties); + reconcilePoolCleaner(properties); + } + /** * 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 diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index 84e37a34cb..d0d867513d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -650,11 +650,12 @@ public class DataSourceProxy implements PoolConfiguration { if (p == null) { return; } - // Make sure the pool is properly configured, on the configuration the - // calling setter wrote, and then start, stop or re-arm the cleaner to match - // what that correction left the configuration asking for. - p.checkPoolConfiguration(poolProperties); - p.reconcilePoolCleaner(poolProperties); + // Validate the configuration the calling setter wrote and bring the + // cleaner in line with what that correction left it asking for, as one + // step under the pool's monitor: the MBean writes the same configuration + // without a monitor, so validating and reconciling in separate calls + // would let its write land between them. + p.applyConfigurationChange(poolProperties); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java index c182557b84..229b405f5b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/ConnectionPool.java @@ -1067,12 +1067,14 @@ public class ConnectionPool extends NotificationBroadcasterSupport implements Co * for the same attributes, and what the pool's creation does when it publishes a * pool. * <p> - * Which half is taken under the pool's monitor, and why neither half is decided - * from a reading this writer took, is recorded at poolCleanerAttributeUpdated. + * Both halves are taken under the pool's monitor, in + * {@link org.apache.tomcat.jdbc.pool.ConnectionPool#applyConfigurationChange(PoolConfiguration)}, + * so a write through the data source cannot land between them. Why neither half + * is decided from a reading this writer took is recorded at + * poolCleanerAttributeUpdated. */ private void poolConfigurationUpdated() { - pool.checkPoolConfiguration(getPoolProperties()); - poolCleanerAttributeUpdated(); + pool.applyConfigurationChange(getPoolProperties()); } @Override diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolSizeSetterValidation.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolSizeSetterValidation.java index 2a54842b8a..bd524cdedc 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolSizeSetterValidation.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolSizeSetterValidation.java @@ -17,8 +17,12 @@ package org.apache.tomcat.jdbc.pool; import java.sql.Connection; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -95,6 +99,50 @@ class TestPoolSizeSetterValidation extends JdbcPoolBaseTest { } } + /* + * Reports whether two configuration checks run at the same time. getMaxAge() + * is called once by checkPoolConfiguration when maxAge is zero, so a barrier + * of two parked in that read is reached only if both writers are inside the + * check together; a serialised pair leaves the first one waiting alone. + */ + private static final class OverlapDetectingProperties extends PoolProperties { + + private static final long serialVersionUID = 1L; + + volatile boolean armed; + final CyclicBarrier barrier = new CyclicBarrier(2); + final AtomicInteger inCheck = new AtomicInteger(); + final AtomicInteger maxConcurrent = new AtomicInteger(); + + @Override + public long getMaxAge() { + if (armed && fromCheck()) { + int now = inCheck.incrementAndGet(); + maxConcurrent.accumulateAndGet(now, Math::max); + try { + barrier.await(500, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } catch (BrokenBarrierException | TimeoutException e) { + // The other writer did not reach the check: serialised + } finally { + inCheck.decrementAndGet(); + } + } + return super.getMaxAge(); + } + + private boolean fromCheck() { + StackTraceElement[] frames = new Throwable().getStackTrace(); + for (StackTraceElement frame : frames) { + if ("checkPoolConfiguration".equals(frame.getMethodName())) { + return true; + } + } + return false; + } + } + /* * The limit that has teeth: sessions are only ever created while the pool is * below maxActive, so a stored zero takes a live pool out of service - the @@ -186,6 +234,66 @@ class TestPoolSizeSetterValidation extends JdbcPoolBaseTest { "precondition: and already bounds the idle count on the active limit"); } + /* + * The two management surfaces write the same configuration: the data source + * holds its own monitor, the MBean holds none, so a check that is not + * serialised with the other writer's check can decide from a state the other + * writer has already left. Both must reach the pool's monitor for the check + * and the cleaner reconciliation, or the repair of one writer is undone by + * the other's reading of an intermediate state. + */ + @Test + void configurationChecksThroughBothSurfacesAreSerialised() throws Exception { + DataSource ds = new DataSource(); + OverlapDetectingProperties properties = new OverlapDetectingProperties(); + properties.setUrl(getUrl()); + properties.setDriverClassName("org.h2.Driver"); + properties.setUsername("sa"); + properties.setPassword(""); + properties.setJmxEnabled(true); + properties.setInitialSize(0); + properties.setMaxActive(4); + properties.setMinIdle(0); + properties.setMaxIdle(4); + ds.setPoolProperties(properties); + manage(ds); + ds.createPool(); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = ds.getPool().getJmxPool(); + Assertions.assertNotNull(jmx, "precondition: a pool created with JMX enabled exposes its MBean"); + + properties.armed = true; + CountDownLatch start = new CountDownLatch(1); + Thread dataSourceWriter = new Thread(() -> { + try { + start.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + ds.setMaxActive(4); + }, "data-source-writer"); + Thread mbeanWriter = new Thread(() -> { + try { + start.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + jmx.setMaxIdle(5); + }, "mbean-writer"); + dataSourceWriter.start(); + mbeanWriter.start(); + start.countDown(); + dataSourceWriter.join(TimeUnit.SECONDS.toMillis(10)); + mbeanWriter.join(TimeUnit.SECONDS.toMillis(10)); + properties.armed = false; + + Assertions.assertEquals(1, properties.maxConcurrent.get(), + "a configuration check on one management surface must not run inside the" + + " check of the other: the validation and the cleaner reconciliation" + + " are one step under the pool's monitor"); + } + /* * The creation-window instance of the same contract. A count writer that * holds nothing can land its store inside the publication check - after --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
