From 26bbb7cfaebc37bcd323b4928efb46b4575938ea Mon Sep 17 00:00:00 2001
From: Mihai Chezan <mihai.chezan@softgress.com>
Date: Sun, 21 Sep 2014 23:23:22 +0300
Subject: [PATCH] Close connectons async. Why: When db goes down, it can cause
 HikariCP to block on getConnection more than the allowed connectionTimeout,
 depending on jdbc driver timeout setting. In some cases, this could be a long
 time. Added a test that shows this behaviour. The test will fail w/o the
 changes to HikariPool.

---
 .../com/zaxxer/hikari/pool/HikariPool.java    | 10 +--
 .../hikari/TestConnectionCloseBlocking.java   | 74 +++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)
 create mode 100644 hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionCloseBlocking.java

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 9dfe9b73..22a78571 100644
--- a/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java
+++ b/hikaricp/src/main/java/com/zaxxer/hikari/pool/HikariPool.java
@@ -76,6 +76,7 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener
    private final HikariConfig configuration;
    private final ConcurrentBag<PoolBagEntry> connectionBag;
    private final ThreadPoolExecutor addConnectionExecutor;
+   private final ThreadPoolExecutor closeConnectionExecutor;
    private final IMetricsTracker metricsTracker;
 
    private final AtomicReference<Throwable> lastConnectionFailure;
@@ -141,7 +142,8 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener
       }
 
       addConnectionExecutor = createThreadPoolExecutor(configuration.getMaximumPoolSize(), "HikariCP connection filler", configuration.getThreadFactory());
-
+      closeConnectionExecutor = createThreadPoolExecutor(configuration.getMaximumPoolSize(), "HikariCP connection closer", configuration.getThreadFactory());
+      
       fillPool();
 
       long delayPeriod = Long.getLong("com.zaxxer.hikari.housekeeping.periodMs", TimeUnit.SECONDS.toMillis(30L));
@@ -239,6 +241,7 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener
          logPoolState("Before shutdown ");
          houseKeepingExecutorService.shutdownNow();
          addConnectionExecutor.shutdownNow();
+         closeConnectionExecutor.shutdownNow();
 
          final long start = System.currentTimeMillis();
          do {
@@ -367,10 +370,7 @@ public final class HikariPool implements HikariPoolMBean, IBagStateListener
          if (tc < 0) {
             LOGGER.warn("Internal accounting inconsistency, totalConnections={}", tc, new Exception());
          }
-         bagEntry.connection.close();
-      }
-      catch (SQLException e) {
-         return;
+         closeConnectionExecutor.submit(() -> { quietlyCloseConnection(bagEntry.connection); });
       }
       finally {
          connectionBag.remove(bagEntry);
diff --git a/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionCloseBlocking.java b/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionCloseBlocking.java
new file mode 100644
index 00000000..0e38c028
--- /dev/null
+++ b/hikaricp/src/test/java/com/zaxxer/hikari/TestConnectionCloseBlocking.java
@@ -0,0 +1,74 @@
+/**
+ * 
+ */
+package com.zaxxer.hikari;
+
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.when;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import com.zaxxer.hikari.mocks.MockDataSource;
+import com.zaxxer.hikari.util.PoolUtilities;
+
+/**
+ * Test for cases when db network connectivity goes down and close is called on existing connections. By default Hikari
+ * blocks longer than getMaximumTimeout (it can hang for a lot of time depending on driver timeout settings). Closing
+ * async the connections fixes this issue.
+ *
+ */
+public class TestConnectionCloseBlocking {
+   @Test
+   public void testConnectionCloseBlocking() throws SQLException {
+      HikariConfig config = new HikariConfig();
+      config.setMinimumIdle(0);
+      config.setMaximumPoolSize(1);
+      config.setConnectionTimeout(1500);
+      config.setDataSource(new CustomMockDataSource());
+
+      HikariDataSource ds = new HikariDataSource(config);
+
+      long start = System.currentTimeMillis();
+      try {
+         Connection connection = ds.getConnection();
+         connection.close();
+         // Hikari only checks for validity for connections with lastAccess > 1000 ms so we sleep for 1001 ms to force
+         // Hikari to do a connection validation which will fail and will trigger the connection to be closed
+         PoolUtilities.quietlySleep(1001);
+         start = System.currentTimeMillis();
+         connection = ds.getConnection(); // on physical connection close we sleep 2 seconds
+         Assert.assertTrue("Waited longer than timeout",
+               (PoolUtilities.elapsedTimeMs(start) < config.getConnectionTimeout()));
+      } catch (SQLException e) {
+         Assert.assertTrue("getConnection failed because close connection took longer than timeout",
+               (PoolUtilities.elapsedTimeMs(start) < config.getConnectionTimeout()));
+      } finally {
+         ds.close();
+      }
+   }
+
+   private static class CustomMockDataSource extends MockDataSource {
+      @Override
+      public Connection getConnection() throws SQLException {
+         Connection mockConnection = super.getConnection();
+         when(mockConnection.isValid(anyInt())).thenReturn(false);
+         doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocation) throws Throwable {
+               TimeUnit.SECONDS.sleep(2);
+               return null;
+            }
+         }).when(mockConnection).close();
+         return mockConnection;
+      }
+   }
+
+}