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 bf2ff77bf22ee4885a6f5f2ee91c0b0d59bccfc2 Author: opencode <[email protected]> AuthorDate: Fri Nov 6 08:56:31 2026 +0100 Close the fresh pool when publication fails A failure in the post-construction configuration validation or cleaner reconciliation left the freshly built pool, its pre-loaded sessions and its cleaner unreachable. The publication step now closes the fresh pool before reporting the failure as an SQLException, and publishes the pool only after both calls succeed. --- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 68 +++++++++++--------- .../TestDataSourceProxyPublicationCleanup.java | 73 ++++++++++++++++++++++ 2 files changed, 112 insertions(+), 29 deletions(-) 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 8de9d82641..f504e9f52f 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 @@ -30,6 +30,7 @@ import javax.sql.XAConnection; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorDefinition; +import org.apache.tomcat.jdbc.pool.util.ExceptionUtils; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** @@ -242,36 +243,45 @@ public class DataSourceProxy implements PoolConfiguration { // anything tears its pool down itself before throwing. final ConnectionPool fresh = new ConnectionPool(poolProperties); final boolean rejected; - synchronized (this) { - // Check-and-publish against the closure, which sets the flag - // and takes the field in one step under this same monitor: - // whichever of the two runs first wins, so the close either - // finds this pool published and closes it, or it found the - // field empty and this thread takes the pool down below. It - // is never neither - that was the leak - and never both. - rejected = closed; - if (!rejected) { - // Finish the configuration before the pool can be read from. - // A write of one of the attributes the check corrects - the - // eviction interval is brought down to the max age - lands its - // value in the shared configuration but skips that correction, - // because the setters run it only once a pool is reachable. The - // publication is where such a write has to be picked up, and it - // has to come before the cleaner is looked over: the check is - // what decides the interval the cleaner should be running on. - fresh.checkPoolConfiguration(poolProperties); - pool = fresh; - // Look the cleaner over now that the pool is reachable. - // It was armed while this pool was still under - // construction, and a sweeper write that landed between - // that arming's reading and this publication found no - // pool to report itself to: the setters take this same - // monitor, so a write is either fully before this point, - // where the arming had not read it yet and this does, or - // fully after it, where the setter finds the pool and - // re-arms itself. Nothing is left in between. - fresh.reconcilePoolCleaner(poolProperties); + try { + synchronized (this) { + // Check-and-publish against the closure, which sets the flag + // and takes the field in one step under this same monitor: + // whichever of the two runs first wins, so the close either + // finds this pool published and closes it, or it found the + // field empty and this thread takes the pool down below. It + // is never neither - that was the leak - and never both. + rejected = closed; + if (!rejected) { + // Finish the configuration before the pool can be read + // from. A write of one of the attributes the check + // corrects lands its value in the shared configuration + // but skips that correction, because the setters run it + // only once a pool is reachable. The check has to come + // before the cleaner is looked over: it decides the + // interval the cleaner should be running on. + fresh.checkPoolConfiguration(poolProperties); + fresh.reconcilePoolCleaner(poolProperties); + pool = fresh; + } + } + } catch (Throwable x) { + // What handleThrowable lets through is not recoverable, so + // check that first. Otherwise the constructor has already + // pre-loaded sessions and may have armed the cleaner, so close + // the fresh pool before reporting the publication failure. + ExceptionUtils.handleThrowable(x); + try { + fresh.close(true); + } catch (Exception y) { + log.warn(MessageFactory.get("dataSourceProxy.errorClosingPool"), y); + } + if (x instanceof SQLException) { + throw (SQLException) x; } + SQLException sx = new SQLException(x.getMessage()); + sx.initCause(x); + throw sx; } if (rejected) { // close(boolean) completed while this pool was being built diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyPublicationCleanup.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyPublicationCleanup.java new file mode 100644 index 0000000000..d2319345f9 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyPublicationCleanup.java @@ -0,0 +1,73 @@ +/* + * 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 java.sql.SQLException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A failure during the post-construction publication step must close the + * freshly built pool, including the physical sessions its initial pre-load + * established. + */ +class TestDataSourceProxyPublicationCleanup extends JdbcPoolBaseTest { + + /** + * Throws when the configuration validation is called directly from the + * publication step, after the pool constructor has already run. + */ + public static class FailingPublicationProperties extends PoolProperties { + + @Override + public int getMaxActive() { + StackTraceElement[] stack = Thread.currentThread().getStackTrace(); + for (int i = 0; i < stack.length - 1; i++) { + if ("checkPoolConfiguration".equals(stack[i].getMethodName()) && + "pCreatePool".equals(stack[i + 1].getMethodName())) { + throw new IllegalStateException("boom from publication"); + } + } + return super.getMaxActive(); + } + } + + @Test + void aFailureDuringPublicationClosesTheFreshPool() throws Exception { + FailingPublicationProperties properties = new FailingPublicationProperties(); + properties.setUrl(getUrl()); + properties.setDriverClassName("org.h2.Driver"); + properties.setUsername("sa"); + properties.setPassword(""); + properties.setJmxEnabled(false); + properties.setInitialSize(1); + properties.setMinIdle(0); + properties.setMaxActive(1); + properties.setMaxIdle(1); + properties.setTimeBetweenEvictionRunsMillis(-1); + DataSource ds = manage(new DataSource(properties)); + + SQLException thrown = Assertions.assertThrows(SQLException.class, ds::createPool, + "a publication failure must fail pool creation"); + Assertions.assertInstanceOf(IllegalStateException.class, thrown.getCause(), + "the publication failure must remain the cause"); + awaitSessions(0); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
