From 3aa1680a2bf450b1780f0d001fef28200e89cae8 Mon Sep 17 00:00:00 2001 From: Gabriel Roldan Date: Tue, 5 May 2026 13:07:51 -0300 Subject: [PATCH] Do not close JNDI-resolved DataSource in JDBCQuotaStore.close() JNDI lookups return a DataSource owned by the JNDI provider; consumers must not close it, since the same name can be (and typically is) shared with other parts of the application. Add an ownsDataSource flag (default true to keep the previous behavior for direct setDataSource callers) that JDBCQuotaStoreFactory clears whenever the configuration uses a JNDISource. on-behalf-of: @camptocamp --- .../diskquota/jdbc/JDBCQuotaStore.java | 39 ++++++++++++-- .../diskquota/jdbc/JDBCQuotaStoreFactory.java | 3 ++ .../jdbc/JDBCQuotaStoreFactoryTest.java | 53 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java b/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java index bcd9cee3c..18e9da7f7 100644 --- a/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java +++ b/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java @@ -91,6 +91,14 @@ public class JDBCQuotaStore implements QuotaStore { private DataSource dataSource; + /** + * Whether {@link #close()} should close {@link #dataSource}. Set to {@code false} by {@link JDBCQuotaStoreFactory} + * when the DataSource is resolved from JNDI - in that case its lifecycle is owned by the JNDI provider, not by this + * store. Defaults to {@code true} so callers using only {@link #setDataSource(DataSource)} keep the previous + * behavior. + */ + private boolean ownsDataSource = true; + public JDBCQuotaStore(DefaultStorageFinder finder, TilePageCalculator tilePageCalculator) { this.finder = finder; this.calculator = tilePageCalculator; @@ -117,6 +125,24 @@ public void setSchema(String schema) { this.schema = schema; } + /** + * @return whether {@link #close()} will close the {@link #setDataSource(DataSource) DataSource}. Defaults to + * {@code true}; set to {@code false} for JNDI-resolved DataSources, whose lifecycle belongs to the JNDI + * provider. + */ + public boolean isOwnsDataSource() { + return ownsDataSource; + } + + /** + * Controls whether {@link #close()} will close the configured {@link #setDataSource(DataSource) DataSource}. Set to + * {@code false} when the DataSource is borrowed (typically resolved from JNDI) so this store does not shut down a + * connection pool it does not own. + */ + public void setOwnsDataSource(boolean ownsDataSource) { + this.ownsDataSource = ownsDataSource; + } + /** Sets the connection pool provider and initializes the tables in the dbms if missing */ @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE") public void setDataSource(DataSource dataSource) { @@ -652,11 +678,14 @@ public PageStats setTruncated(final TilePage page) throws InterruptedException { public void close() throws Exception { log.info("Closing up the JDBC quota store "); - // try to close the data source if possible - if (dataSource instanceof BasicDataSource source) { - source.close(); - } else if (dataSource instanceof Closeable closeable) { - closeable.close(); + // try to close the data source if possible, but only if we own it - JNDI-resolved + // DataSources are owned by the JNDI provider and must not be closed here. + if (ownsDataSource) { + if (dataSource instanceof BasicDataSource source) { + source.close(); + } else if (dataSource instanceof Closeable closeable) { + closeable.close(); + } } // release the templates diff --git a/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactory.java b/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactory.java index 2d308d41a..4a6e91cd1 100644 --- a/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactory.java +++ b/geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactory.java @@ -161,6 +161,9 @@ private QuotaStore getJDBCStore( store.setDialect(dialect); // sets schema if configured in geowebcache-diskquota-jdbc.xml store.setSchema(expandedConfig.getSchema()); + // JNDI-resolved DataSources are owned by the JNDI provider; inline connection pools are + // created here and owned by the store. + store.setOwnsDataSource(expandedConfig.getJNDISource() == null); // initialize it store.initialize(); diff --git a/geowebcache/diskquota/jdbc/src/test/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactoryTest.java b/geowebcache/diskquota/jdbc/src/test/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactoryTest.java index 0f1ac4754..644c12840 100644 --- a/geowebcache/diskquota/jdbc/src/test/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactoryTest.java +++ b/geowebcache/diskquota/jdbc/src/test/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactoryTest.java @@ -14,14 +14,19 @@ package org.geowebcache.diskquota.jdbc; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.Closeable; import java.sql.Connection; import javax.naming.InitialContext; import javax.sql.DataSource; import org.geotools.util.factory.GeoTools; import org.geowebcache.config.ConfigurationException; +import org.geowebcache.diskquota.storage.TilePageCalculator; +import org.geowebcache.storage.DefaultStorageFinder; import org.junit.Test; import org.mockito.Mockito; @@ -60,4 +65,52 @@ public void testJNDILookup() throws Exception { GeoTools.clearInitialContext(); } } + + /** A {@link DataSource} that also implements {@link Closeable} so we can observe whether close() is called. */ + private interface CloseableDataSource extends DataSource, Closeable {} + + @Test + @SuppressWarnings({"PMD.CloseResource"}) + public void closeDoesNotCloseJndiDataSource() throws Exception { + CloseableDataSource jndiDs = Mockito.mock(CloseableDataSource.class); + + JDBCQuotaStore store = + new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class)); + store.setDataSource(jndiDs); + // Mirror the wiring done by JDBCQuotaStoreFactory#getJDBCStore for a JNDI configuration. + store.setOwnsDataSource(false); + + store.close(); + + assertFalse("ownsDataSource flag must be false after explicit set", store.isOwnsDataSource()); + Mockito.verify(jndiDs, Mockito.never()).close(); + } + + @Test + @SuppressWarnings({"PMD.CloseResource"}) + public void closeClosesOwnedDataSource() throws Exception { + CloseableDataSource ownedDs = Mockito.mock(CloseableDataSource.class); + + JDBCQuotaStore store = + new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class)); + store.setDataSource(ownedDs); + // Default ownsDataSource=true preserves the historical behavior. + assertTrue(store.isOwnsDataSource()); + + store.close(); + + Mockito.verify(ownedDs).close(); + } + + @Test + public void closeIsSafeWhenDataSourceLacksCloseable() throws Exception { + DataSource plainDs = Mockito.mock(DataSource.class); + JDBCQuotaStore store = + new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class)); + store.setDataSource(plainDs); + + // No assertion beyond the absence of an exception - the store should not try to close a + // DataSource that does not implement Closeable / BasicDataSource. + store.close(); + } }