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(); + } }