From e6fb2a4e72fc8c81bacc913d9cec69d54d166d34 Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Sun, 12 Oct 2014 19:31:14 +0900 Subject: [PATCH] Fix shutdown race with connections added to the pool. --- .../java/com/zaxxer/hikari/pool/HikariPool.java | 1 + .../java/com/zaxxer/hikari/util/ConcurrentBag.java | 13 +++++++++++++ .../test/java/com/zaxxer/hikari/ShutdownTest.java | 2 +- .../zaxxer/hikari/TestConnectionTimeoutRetry.java | 2 +- .../java/com/zaxxer/hikari/pool/HikariPool.java | 1 + .../java/com/zaxxer/hikari/util/ConcurrentBag.java | 13 +++++++++++++ .../test/java/com/zaxxer/hikari/ShutdownTest.java | 2 +- .../zaxxer/hikari/TestConnectionTimeoutRetry.java | 2 +- 8 files changed, 32 insertions(+), 4 deletions(-) diff --git a/hikaricp-java6/src/main/java/com/zaxxer/hikari/pool/HikariPool.java b/hikaricp-java6/src/main/java/com/zaxxer/hikari/pool/HikariPool.java index b1cd5abc..ae5fca5e 100644 --- a/hikaricp-java6/src/main/java/com/zaxxer/hikari/pool/HikariPool.java +++ b/hikaricp-java6/src/main/java/com/zaxxer/hikari/pool/HikariPool.java @@ -250,6 +250,7 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener isShutdown = true; LOGGER.info("HikariCP pool {} is shutting down.", configuration.getPoolName()); + connectionBag.close(); logPoolState("Before shutdown "); houseKeepingExecutorService.shutdownNow(); addConnectionExecutor.shutdownNow(); diff --git a/hikaricp-java6/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java b/hikaricp-java6/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java index d2458fbb..321bc0cd 100644 --- a/hikaricp-java6/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java +++ b/hikaricp-java6/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java @@ -81,6 +81,7 @@ public final class ConcurrentBag private final Synchronizer synchronizer; private final AtomicLong sequence; private final IBagStateListener listener; + private volatile boolean closed; /** * Construct a ConcurrentBag with the specified listener. @@ -179,6 +180,10 @@ public final class ConcurrentBag */ public void add(final T bagEntry) { + if (closed) { + throw new IllegalStateException("ConcurrentBag has been closed"); + } + sharedList.add(bagEntry); synchronizer.releaseShared(sequence.incrementAndGet()); } @@ -203,6 +208,14 @@ public final class ConcurrentBag } } + /** + * Close the bag to further adds. + */ + public void close() + { + closed = true; + } + /** * This method provides a "snaphot" in time of the BagEntry * items in the bag in the specified state. It does not "lock" diff --git a/hikaricp-java6/src/test/java/com/zaxxer/hikari/ShutdownTest.java b/hikaricp-java6/src/test/java/com/zaxxer/hikari/ShutdownTest.java index 6f31a77f..6d626e9f 100644 --- a/hikaricp-java6/src/test/java/com/zaxxer/hikari/ShutdownTest.java +++ b/hikaricp-java6/src/test/java/com/zaxxer/hikari/ShutdownTest.java @@ -167,7 +167,7 @@ public class ShutdownTest PoolUtilities.quietlySleep(250); } - Assert.assertSame("Thread was leaked", 0, threadCount()); + Assert.assertSame("Unreleased connections after shutdown", 0, TestElf.getPool(ds).getTotalConnections()); } @Test diff --git a/hikaricp-java6/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java b/hikaricp-java6/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java index 3f02abbe..b01c9e00 100644 --- a/hikaricp-java6/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java +++ b/hikaricp-java6/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java @@ -253,7 +253,7 @@ public class TestConnectionTimeoutRetry Connection connection6 = ds.getConnection(); Connection connection7 = ds.getConnection(); - Thread.sleep(1250); + Thread.sleep(1350); Assert.assertSame("Totals connections not as expected", 10, TestElf.getPool(ds).getTotalConnections()); Assert.assertSame("Idle connections not as expected", 3, TestElf.getPool(ds).getIdleConnections()); diff --git a/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java b/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java index 7a2c3abf..71204254 100644 --- a/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java +++ b/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java @@ -247,6 +247,7 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener isShutdown = true; LOGGER.info("HikariCP pool {} is shutting down.", configuration.getPoolName()); + connectionBag.close(); logPoolState("Before shutdown "); houseKeepingExecutorService.shutdownNow(); addConnectionExecutor.shutdownNow(); diff --git a/hikaricp/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java b/hikaricp/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java index 8bde80bc..9d691364 100644 --- a/hikaricp/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java +++ b/hikaricp/src/main/java/com/zaxxer/hikari/util/ConcurrentBag.java @@ -80,6 +80,7 @@ public final class ConcurrentBag private final Synchronizer synchronizer; private final AtomicLong sequence; private final IBagStateListener listener; + private volatile boolean closed; /** * Construct a ConcurrentBag with the specified listener. @@ -178,6 +179,10 @@ public final class ConcurrentBag */ public void add(final T bagEntry) { + if (closed) { + throw new IllegalStateException("ConcurrentBag has been closed"); + } + sharedList.add(bagEntry); synchronizer.releaseShared(sequence.incrementAndGet()); } @@ -202,6 +207,14 @@ public final class ConcurrentBag } } + /** + * Close the bag to further adds. + */ + public void close() + { + closed = true; + } + /** * This method provides a "snaphot" in time of the BagEntry * items in the bag in the specified state. It does not "lock" diff --git a/hikaricp/src/test/java/com/zaxxer/hikari/ShutdownTest.java b/hikaricp/src/test/java/com/zaxxer/hikari/ShutdownTest.java index 6f31a77f..6d626e9f 100644 --- a/hikaricp/src/test/java/com/zaxxer/hikari/ShutdownTest.java +++ b/hikaricp/src/test/java/com/zaxxer/hikari/ShutdownTest.java @@ -167,7 +167,7 @@ public class ShutdownTest PoolUtilities.quietlySleep(250); } - Assert.assertSame("Thread was leaked", 0, threadCount()); + Assert.assertSame("Unreleased connections after shutdown", 0, TestElf.getPool(ds).getTotalConnections()); } @Test diff --git a/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java b/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java index 3f02abbe..b01c9e00 100644 --- a/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java +++ b/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionTimeoutRetry.java @@ -253,7 +253,7 @@ public class TestConnectionTimeoutRetry Connection connection6 = ds.getConnection(); Connection connection7 = ds.getConnection(); - Thread.sleep(1250); + Thread.sleep(1350); Assert.assertSame("Totals connections not as expected", 10, TestElf.getPool(ds).getTotalConnections()); Assert.assertSame("Idle connections not as expected", 3, TestElf.getPool(ds).getIdleConnections());