From 48db08659ead50d63b9d72b493d5a59234cbbf0e Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:25:10 -0600 Subject: [PATCH 1/2] Preserve SQL pool ownership on connection acquisition failure --- .../simpleapi/sql/mysql/AbstractSqlTable.java | 10 +- .../sql/mysql/ConnectionManager.java | 92 +++++++--- .../simpleapi/sql/mysql/queries/Query.java | 6 +- .../sql/mysql/ConnectionManagerPoolTest.java | 164 ++++++++++++++++++ docs/shared-libraries.md | 18 ++ 5 files changed, 254 insertions(+), 36 deletions(-) create mode 100644 SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java diff --git a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/AbstractSqlTable.java b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/AbstractSqlTable.java index 57810f8b..36a96c20 100644 --- a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/AbstractSqlTable.java +++ b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/AbstractSqlTable.java @@ -207,7 +207,7 @@ public boolean containsKey(String key) { public boolean containsKeyQuery(String key) { String sql = "SELECT 1 FROM " + qi(tableName) + " WHERE " + qi(getPrimaryKeyColumn()) + " = ? LIMIT 1;"; - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement ps = conn.prepareStatement(sql)) { ps.setString(1, key); try (ResultSet rs = ps.executeQuery()) { @@ -333,7 +333,7 @@ public void alterColumnType(String column, String newType) { * FIXED: no external Connection needed. */ public boolean columnNeedsAlter(String column, String newType) throws SQLException { - try (Connection conn = mysql.getConnectionManager().getConnection()) { + try (Connection conn = mysql.getConnectionManager().getConnectionChecked()) { if (dbType == DbType.POSTGRESQL) { return columnNeedsAlterPostgres(conn, column, newType); } @@ -530,7 +530,7 @@ public final void loadBasicCaches() { public List getPrimaryKeysQuery() { List keys = new ArrayList<>(); String sql = "SELECT " + qi(getPrimaryKeyColumn()) + " FROM " + qi(tableName) + ";"; - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement ps = conn.prepareStatement(sql); ResultSet rs = ps.executeQuery()) { while (rs.next()) { @@ -550,7 +550,7 @@ public List getColumnsQuery() { if (dbType == DbType.POSTGRESQL) { String sql = "SELECT column_name FROM information_schema.columns " + "WHERE table_schema = current_schema() AND table_name = ?;"; - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement ps = conn.prepareStatement(sql)) { ps.setString(1, tableName); try (ResultSet rs = ps.executeQuery()) { @@ -565,7 +565,7 @@ public List getColumnsQuery() { } String sql = "SHOW COLUMNS FROM " + qi(tableName) + ";"; - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement ps = conn.prepareStatement(sql); ResultSet rs = ps.executeQuery()) { while (rs.next()) { diff --git a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java index d1639056..d14382db 100644 --- a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java +++ b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java @@ -30,8 +30,7 @@ public class ConnectionManager { private String database; @Getter - @Setter - private HikariDataSource dataSource; + private volatile HikariDataSource dataSource; @Getter @Setter @@ -141,29 +140,65 @@ public ConnectionManager(String host, String port, String username, String passw this.dbType = useMariaDB ? DbType.MARIADB : DbType.MYSQL; } - public boolean isClosed() { - return dataSource == null || dataSource.isClosed(); - } - - public void close() { - if (!isClosed()) { - dataSource.close(); - } - } - - public Connection getConnection() { - try { - if (isClosed()) { - open(); - } - return dataSource.getConnection(); - } catch (SQLException e) { - e.printStackTrace(); - open(); - return null; - } - } - + /** Transfers ownership of a supplied pool and retires the previous one. */ + public synchronized void setDataSource(HikariDataSource replacement) { + replaceDataSource(replacement); + } + + private void replaceDataSource(HikariDataSource replacement) { + HikariDataSource predecessor = dataSource; + if (predecessor == replacement) return; + dataSource = replacement; + if (predecessor != null) { + try { + predecessor.close(); + } catch (RuntimeException cleanupFailure) { + // Publication succeeded; cleanup failure must not misreport initialization failure. + System.err.println("Unable to close predecessor SQL pool after replacement"); + } + } + } + + public boolean isClosed() { + HikariDataSource current = dataSource; + return current == null || current.isClosed(); + } + + public synchronized void close() { + if (!isClosed()) { + dataSource.close(); + } + } + + /** + * Legacy no-checked-exception entry point. Acquisition failure is explicit, + * never a null connection; SQL callers should use getConnectionChecked(). + */ + public Connection getConnection() { + try { + return getConnectionChecked(); + } catch (SQLException failure) { + throw new IllegalStateException("Unable to obtain a SQL pool connection", failure); + } + } + + /** + * Borrows from the current pool without replacing it on timeout/exhaustion. + * The lifecycle monitor is released before waiting for a lease, so shutdown + * does not wait for an exhausted borrow's connection timeout. + * @throws SQLException when initialization or acquisition fails + */ + public Connection getConnectionChecked() throws SQLException { + HikariDataSource current; + synchronized (this) { + if (isClosed() && !open()) { + throw new SQLException("Unable to initialize SQL connection pool"); + } + current = dataSource; + } + return current.getConnection(); + } + private void ensureDriverPresent(String className) throws ClassNotFoundException { Class.forName(className); } @@ -291,7 +326,7 @@ private void rejectConflictingPostgreSqlTlsOptions(String extra) { // --- Pool Configuration --- - public boolean open() { + public synchronized boolean open() { try { // If someone only set legacy flag but not dbType explicitly, keep it consistent if (dbType == null) { @@ -342,8 +377,9 @@ public boolean open() { cfg.setAutoCommit(true); cfg.setPoolName(poolName != null && !poolName.isEmpty() ? poolName : "SimpleAPI-Hikari"); - dataSource = new HikariDataSource(cfg); - return true; + HikariDataSource replacement = new HikariDataSource(cfg); + replaceDataSource(replacement); + return true; } catch (Exception e) { e.printStackTrace(); return false; diff --git a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/queries/Query.java b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/queries/Query.java index 9b87a678..d939e9a8 100644 --- a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/queries/Query.java +++ b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/queries/Query.java @@ -42,7 +42,7 @@ public void addBatch() throws SQLException { * @throws SQLException SQLException */ public int[] executeBatch() throws SQLException { - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement sql = conn.prepareStatement(this.sql);) { for (Entry entry : paramters.entrySet()) { @@ -107,7 +107,7 @@ public void run() { * @Deprecated public ResultSet executeQuery() throws SQLException { * CachedRowSet rowSet = RowSetProvider.newFactory().createCachedRowSet(); * ResultSet resultSet = null; try (Connection conn = - * mysql.getConnectionManager().getConnection(); PreparedStatement sql = + * mysql.getConnectionManager().getConnectionChecked(); PreparedStatement sql = * conn.prepareStatement(this.sql);) { for (Entry entry : * paramters.entrySet()) { sql.setObject(entry.getKey(), entry.getValue()); } if * (addBatch) { sql.addBatch(); } resultSet = sql.executeQuery(); @@ -139,7 +139,7 @@ public void run() { * @throws SQLException SQLException */ public int executeUpdate() throws SQLException { - try (Connection conn = mysql.getConnectionManager().getConnection(); + try (Connection conn = mysql.getConnectionManager().getConnectionChecked(); PreparedStatement sql = conn.prepareStatement(this.sql);) { for (Entry entry : paramters.entrySet()) { sql.setObject(entry.getKey(), entry.getValue()); diff --git a/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java b/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java new file mode 100644 index 00000000..ec05e4b2 --- /dev/null +++ b/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java @@ -0,0 +1,164 @@ +package com.bencodez.simpleapi.sql.mysql; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; +import java.sql.*; +import java.util.Properties; +import java.util.concurrent.*; +import java.util.logging.Logger; +import com.zaxxer.hikari.HikariDataSource; +import org.junit.jupiter.api.Test; + +class ConnectionManagerPoolTest { + @Test void borrowTimeoutKeepsPoolAndRecoversAfterLeasesAreReleased() throws Exception { + for (DbType type : DbType.values()) verifyBorrowRecovery(type); + } + + private void verifyBorrowRecovery(DbType type) throws Exception { + ConnectionManager manager = manager(type); + HikariDataSource pool = mock(HikariDataSource.class); + setPool(manager, pool); + SQLException exhausted = new SQLTransientConnectionException("controlled pool exhaustion"); + Connection recovered = mock(Connection.class); + when(pool.getConnection()).thenThrow(exhausted).thenThrow(exhausted).thenReturn(recovered); + assertSame(exhausted, assertThrows(SQLException.class, manager::getConnectionChecked)); + IllegalStateException legacy = assertThrows(IllegalStateException.class, manager::getConnection); + assertSame(exhausted, legacy.getCause()); + assertSame(pool, manager.getDataSource()); + verify(pool, never()).close(); + assertSame(recovered, manager.getConnectionChecked()); + } + + @Test void explicitReplacementClosesPredecessorAndShutdownClosesSuccessor() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + try (var pools = mockConstruction(HikariDataSource.class)) { + assertTrue(manager.open()); + HikariDataSource first = pools.constructed().get(0); + assertTrue(manager.open()); + HikariDataSource second = pools.constructed().get(1); + assertSame(second, manager.getDataSource()); + verify(first).close(); + verify(second, never()).close(); + manager.close(); + verify(second).close(); + } + } + + @Test void publicPoolSetterRetiresPredecessorWithoutClosingSamePool() { + ConnectionManager manager = manager(DbType.MYSQL); + HikariDataSource first = mock(HikariDataSource.class); + HikariDataSource second = mock(HikariDataSource.class); + manager.setDataSource(first); + manager.setDataSource(first); + verify(first, never()).close(); + manager.setDataSource(second); + verify(first).close(); + verify(second, never()).close(); + manager.setDataSource(null); + verify(second).close(); + assertTrue(manager.isClosed()); + } + + @Test void failedReplacementKeepsWorkingPredecessor() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + HikariDataSource old = mock(HikariDataSource.class); + Connection available = mock(Connection.class); + when(old.getConnection()).thenReturn(available); + setPool(manager, old); + try (var pools = mockConstruction(HikariDataSource.class, (pool, context) -> { + throw new IllegalStateException("controlled initialization failure"); + })) { + assertFalse(manager.open()); + assertSame(old, manager.getDataSource()); + verify(old, never()).close(); + assertSame(available, manager.getConnectionChecked()); + } + } + + @Test void initializationFailureNeverReturnsNull() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + manager.setMysqlDriver("missing.probe.Driver"); + assertThrows(SQLException.class, manager::getConnectionChecked); + assertThrows(IllegalStateException.class, manager::getConnection); + assertNull(manager.getDataSource()); + } + + @Test void closedPoolIsReopenedAndConnectionIsReturned() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + HikariDataSource closed = mock(HikariDataSource.class); + when(closed.isClosed()).thenReturn(true); + setPool(manager, closed); + Connection connection = mock(Connection.class); + try (var pools = mockConstruction(HikariDataSource.class, + (pool, context) -> when(pool.getConnection()).thenReturn(connection))) { + assertSame(connection, manager.getConnectionChecked()); + assertSame(pools.constructed().get(0), manager.getDataSource()); + verify(closed).close(); + } + } + + @Test void blockedBorrowDoesNotHoldLifecycleMonitor() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + HikariDataSource pool = mock(HikariDataSource.class); + setPool(manager, pool); + CountDownLatch borrowed = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + when(pool.getConnection()).thenAnswer(invocation -> { + borrowed.countDown(); + if (!release.await(5, TimeUnit.SECONDS)) throw new AssertionError("release not signalled"); + throw new SQLTransientConnectionException("pool closed while waiting"); + }); + ExecutorService workers = Executors.newFixedThreadPool(2); + try { + Future borrow = workers.submit(() -> assertThrows(SQLException.class, manager::getConnectionChecked)); + assertTrue(borrowed.await(5, TimeUnit.SECONDS)); + workers.submit(manager::close).get(5, TimeUnit.SECONDS); + verify(pool).close(); + release.countDown(); + borrow.get(5, TimeUnit.SECONDS); + assertSame(pool, manager.getDataSource()); + } finally { + release.countDown(); workers.shutdownNow(); + } + } + + @Test void predecessorCleanupFailureDoesNotMisreportPublishedSuccessor() throws Exception { + ConnectionManager manager = manager(DbType.MYSQL); + HikariDataSource old = mock(HikariDataSource.class); + doThrow(new IllegalStateException("controlled cleanup failure")).when(old).close(); + setPool(manager, old); + try (var pools = mockConstruction(HikariDataSource.class)) { + assertTrue(manager.open()); + assertSame(pools.constructed().get(0), manager.getDataSource()); + } + } + + @Test void queryAcquisitionFailureUsesExistingSqlFailurePath() throws Exception { + MySQL mysql = mock(MySQL.class); + ConnectionManager manager = mock(ConnectionManager.class); + when(mysql.getConnectionManager()).thenReturn(manager); + when(manager.getConnectionChecked()).thenThrow(new SQLTransientConnectionException("exhausted")); + var query = new com.bencodez.simpleapi.sql.mysql.queries.Query(mysql, "UPDATE example SET value=1"); + assertDoesNotThrow(query::executeUpdate); + verify(manager).getConnectionChecked(); + verify(manager, never()).getConnection(); + } + + private static ConnectionManager manager(DbType type) { + ConnectionManager result = new ConnectionManager("localhost", "3306", "test", "", "test"); + result.setDbType(type); result.setMysqlDriver(TestDriver.class.getName()); + return result; + } + private static void setPool(ConnectionManager manager, HikariDataSource pool) throws Exception { + var field = ConnectionManager.class.getDeclaredField("dataSource"); + field.setAccessible(true); field.set(manager, pool); + } + public static class TestDriver implements Driver { + public Connection connect(String u, Properties p) { return null; } + public boolean acceptsURL(String u) { return true; } + public DriverPropertyInfo[] getPropertyInfo(String u, Properties p) { return new DriverPropertyInfo[0]; } + public int getMajorVersion() { return 1; } public int getMinorVersion() { return 0; } + public boolean jdbcCompliant() { return false; } + public Logger getParentLogger() { return Logger.getGlobal(); } + } +} diff --git a/docs/shared-libraries.md b/docs/shared-libraries.md index b58f540a..9a2a2e09 100644 --- a/docs/shared-libraries.md +++ b/docs/shared-libraries.md @@ -99,6 +99,24 @@ does not. Future mixed AdvancedCore common/Bukkit packaging must align relocatio of callers and implementations and test the final JAR. Exclusions alone do not prove the mixed SQL signatures compatible. Prefer JDBC/JDK types at new boundaries. + +### SQL pool acquisition and lifecycle + +`ConnectionManager.getConnectionChecked()` borrows a lease or throws +`SQLException`; a borrow timeout/exhaustion does not reopen/replace its pool. +SimpleAPI SQL queries/table operations use this checked failure path. The legacy +`getConnection()` signature remains available and throws `IllegalStateException` +with the SQL cause on acquisition failure instead of returning a null connection. +Callers with SQL error handling should prefer the additive checked method. + +Explicit `open()` constructs its successor before publication, retains the +predecessor if initialization fails, and closes the predecessor after a successful +replacement. The existing datasource setter uses the same ownership transfer and +keeps a same-pool assignment unchanged. Lifecycle operations serialize publication/close; waiting for a lease +does not hold that monitor. Existing lazy initialization/reopening remains supported. +All SQL calls are synchronous: keep them on the owning storage worker, not a +platform/server thread. + ## Configuration use and guarantees New code may import `com.bencodez.simpleapi.core.config.AnnotationBinder`, From 31bb908b611fed772d9aedaaa35e36910b7f108b Mon Sep 17 00:00:00 2001 From: BenCodez <17074231+BenCodez@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:17:53 -0600 Subject: [PATCH 2/2] Preserve legacy SQL acquisition and datasource setter contracts --- .../simpleapi/sql/mysql/ConnectionManager.java | 11 ++++++----- .../sql/mysql/ConnectionManagerPoolTest.java | 14 +++++++------- docs/shared-libraries.md | 8 ++++---- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java index d14382db..679477a9 100644 --- a/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java +++ b/SimpleAPI/src/main/java/com/bencodez/simpleapi/sql/mysql/ConnectionManager.java @@ -140,9 +140,9 @@ public ConnectionManager(String host, String port, String username, String passw this.dbType = useMariaDB ? DbType.MARIADB : DbType.MYSQL; } - /** Transfers ownership of a supplied pool and retires the previous one. */ + /** Legacy assignment-only setter; the caller retains predecessor ownership. */ public synchronized void setDataSource(HikariDataSource replacement) { - replaceDataSource(replacement); + dataSource = replacement; } private void replaceDataSource(HikariDataSource replacement) { @@ -171,14 +171,15 @@ public synchronized void close() { } /** - * Legacy no-checked-exception entry point. Acquisition failure is explicit, - * never a null connection; SQL callers should use getConnectionChecked(). + * Legacy no-checked-exception entry point. Preserves null on acquisition + * failure; SQL callers should use getConnectionChecked(). */ public Connection getConnection() { try { return getConnectionChecked(); } catch (SQLException failure) { - throw new IllegalStateException("Unable to obtain a SQL pool connection", failure); + System.err.println("Unable to obtain a SQL pool connection"); + return null; } } diff --git a/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java b/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java index ec05e4b2..1ead066e 100644 --- a/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java +++ b/SimpleAPI/src/test/java/com/bencodez/simpleapi/sql/mysql/ConnectionManagerPoolTest.java @@ -22,8 +22,7 @@ private void verifyBorrowRecovery(DbType type) throws Exception { Connection recovered = mock(Connection.class); when(pool.getConnection()).thenThrow(exhausted).thenThrow(exhausted).thenReturn(recovered); assertSame(exhausted, assertThrows(SQLException.class, manager::getConnectionChecked)); - IllegalStateException legacy = assertThrows(IllegalStateException.class, manager::getConnection); - assertSame(exhausted, legacy.getCause()); + assertNull(manager.getConnection()); assertSame(pool, manager.getDataSource()); verify(pool, never()).close(); assertSame(recovered, manager.getConnectionChecked()); @@ -44,7 +43,7 @@ private void verifyBorrowRecovery(DbType type) throws Exception { } } - @Test void publicPoolSetterRetiresPredecessorWithoutClosingSamePool() { + @Test void publicPoolSetterPreservesCallerOwnership() { ConnectionManager manager = manager(DbType.MYSQL); HikariDataSource first = mock(HikariDataSource.class); HikariDataSource second = mock(HikariDataSource.class); @@ -52,10 +51,11 @@ private void verifyBorrowRecovery(DbType type) throws Exception { manager.setDataSource(first); verify(first, never()).close(); manager.setDataSource(second); - verify(first).close(); + verify(first, never()).close(); verify(second, never()).close(); manager.setDataSource(null); - verify(second).close(); + verify(first, never()).close(); + verify(second, never()).close(); assertTrue(manager.isClosed()); } @@ -75,11 +75,11 @@ private void verifyBorrowRecovery(DbType type) throws Exception { } } - @Test void initializationFailureNeverReturnsNull() throws Exception { + @Test void initializationFailurePreservesLegacyNullAndCheckedFailure() throws Exception { ConnectionManager manager = manager(DbType.MYSQL); manager.setMysqlDriver("missing.probe.Driver"); assertThrows(SQLException.class, manager::getConnectionChecked); - assertThrows(IllegalStateException.class, manager::getConnection); + assertNull(manager.getConnection()); assertNull(manager.getDataSource()); } diff --git a/docs/shared-libraries.md b/docs/shared-libraries.md index 9a2a2e09..a6b4e88c 100644 --- a/docs/shared-libraries.md +++ b/docs/shared-libraries.md @@ -105,14 +105,14 @@ prove the mixed SQL signatures compatible. Prefer JDBC/JDK types at new boundari `ConnectionManager.getConnectionChecked()` borrows a lease or throws `SQLException`; a borrow timeout/exhaustion does not reopen/replace its pool. SimpleAPI SQL queries/table operations use this checked failure path. The legacy -`getConnection()` signature remains available and throws `IllegalStateException` -with the SQL cause on acquisition failure instead of returning a null connection. +`getConnection()` signature preserves its null-on-acquisition-failure contract. Callers with SQL error handling should prefer the additive checked method. Explicit `open()` constructs its successor before publication, retains the predecessor if initialization fails, and closes the predecessor after a successful -replacement. The existing datasource setter uses the same ownership transfer and -keeps a same-pool assignment unchanged. Lifecycle operations serialize publication/close; waiting for a lease +replacement. The existing datasource setter remains assignment-only and never +closes the predecessor supplied by a caller. Lifecycle operations serialize +publication/close; waiting for a lease does not hold that monitor. Existing lazy initialization/reopening remains supported. All SQL calls are synchronous: keep them on the owning storage worker, not a platform/server thread.