Bugfix: Test Connections before Returning them from ConnectionPool.

From the time the connection was originally created, many problems may have occurred, including
a network outage, server restart, timeout, and many other possibilities. In order to prevent
these temporary errors from permanently causing connections returned by the ConnectionPool,
test the connection before returning it to the user.

In order to prevent inordinate delays, e.g. the first connection to be made after 100 connections
had previously been made, only 1 connection will be tested before creating a new one.

Fixes: #124.

Author: vnayar

source/ddbc/common.d

@@ -170,18 +170,40 @@ public:
 		this.waitTimeOut = waitTimeOut;
 	}
 
+    /**
+     * If a previously closed connection is available, test it and return it instead of creating a
+     * new connection. At most, one connection will be tested before creating a new connection, in
+     * order to prevent large delays due to having to test multiple connections.
+     */
 	override Connection getConnection() {
 		Connection conn = null;
-        //writeln("getConnection(): freeConnections.length = " ~ to!string(freeConnections.length));
-        if (freeConnections.length > 0) {
-    				tracef("Retrieving database connection from pool of %s", freeConnections.length);
-            conn = freeConnections[freeConnections.length - 1]; // $ - 1
-            auto oldSize = freeConnections.length;
-            myRemove(freeConnections, freeConnections.length - 1);
+		//writeln("getConnection(): freeConnections.length = " ~ to!string(freeConnections.length));
+		if (freeConnections.length > 0) {
+			tracef("Retrieving database connection from pool of %s", freeConnections.length);
+			conn = freeConnections[freeConnections.length - 1]; // $ - 1
+			auto oldSize = freeConnections.length;
+			myRemove(freeConnections, freeConnections.length - 1);
             //freeConnections.length = oldSize - 1; // some bug in remove? length is not decreased...
             auto newSize = freeConnections.length;
             assert(newSize == oldSize - 1);
-        } else {
+
+            // Test the connection to make sure it is still alive.
+            Statement testStatement = conn.createStatement();
+            scope (exit) testStatement.close();
+            try {
+                ResultSet testResultSet = testStatement.executeQuery("SELECT 1;");
+                scope (exit) testResultSet.close();
+            } catch (Exception e) {
+                // This connection is not usable, do not add it to active connections,
+                // and do not cycle through the rest of the freeConnections to prevent
+                // excess delays.
+                trace("Exception trying to use freeConnection.", e);
+                conn = null;
+            }
+        }
+
+        // Either there were no free connections, or the one that was picked out was invalid.
+        if (conn is null) {
             tracef("Creating new database connection (%s) %s %s", driver, url, params);
 
             try {

test/ddbctest/testconnectionpool.d

@@ -0,0 +1,129 @@
+import std.stdio;
+import std.datetime;
+import std.variant;
+import std.conv;
+
+import core.thread : Thread;
+import core.time : seconds;
+
+import ddbc.drivers.pgsqlddbc;
+import ddbc.core;
+import ddbc.common;
+
+import dunit;
+import ddbc.test.common : DdbcTestFixture;
+import ddbc.core : Connection, PreparedStatement, Statement, SQLException, TransactionIsolation;
+
+// Used to control our fake DB connection and when it throws errors.
+bool throwOnConnect = false;
+int connectCount = 0;
+bool throwOnExecute = false;
+int executeCount = 0;
+
+// A fake query result we can use to simulate errors.
+class FakeResultSet : ResultSetImpl {
+    override
+    void close() { }
+}
+
+// A fake statement we can use to simulate errors on query.
+class FakeStatement : Statement {
+    ResultSet executeQuery(string query) {
+        if (throwOnExecute) {
+            throw new SQLException("Fake execute exception.");
+        }
+        executeCount++;
+        return new FakeResultSet();
+    }
+    int executeUpdate(string query) { return 0; }
+    int executeUpdate(string query, out Variant insertId) { return 0; }
+    void close() { }
+
+    DialectType getDialectType() {
+        return DialectType.SQLITE;  // Just need to pick something.
+    }
+}
+
+class FakeConnection : Connection {
+    void close() { }
+    void commit() { }
+    string getCatalog() { return ""; }
+    void setCatalog(string catalog) { }
+    bool isClosed() { return false; }
+    void rollback() { }
+    bool getAutoCommit() { return false; }
+    void setAutoCommit(bool autoCommit) { }
+    Statement createStatement() { return new FakeStatement(); }
+    PreparedStatement prepareStatement(string query) { return null;}
+    TransactionIsolation getTransactionIsolation() { return TransactionIsolation.READ_COMMITTED; }
+    void setTransactionIsolation(TransactionIsolation level) { }
+
+    DialectType getDialectType() {
+        return DialectType.SQLITE;  // Just need to pick something.
+    }
+}
+
+// A fake driver we can use to simulate failures to connect.
+class FakeDriver : Driver {
+    Connection connect(string url, string[string] params) {
+        if (throwOnConnect) {
+            throw new SQLException("Fake connect exception.");
+        }
+        connectCount++;
+        return new FakeConnection();
+    }
+}
+
+class ConnectionPoolTest {
+    mixin UnitTest;
+
+    @Test
+    public void testBrokenConnection() {
+        Driver driver = new FakeDriver();
+        DataSource dataSource = new ConnectionPoolDataSourceImpl(driver, "");
+
+        // Test verify that when the database is down, nothing can be done.
+        throwOnConnect = true;
+        throwOnExecute = false;
+        try {
+            Connection connection = dataSource.getConnection();
+            assert(false, "Expected exception when no connection can be established.");
+        } catch (Exception e) {
+            // Ignore exception.
+        }
+        assert(connectCount == 0);
+
+        // Obtain a working connection, and validate that it gets recycled.
+        throwOnConnect = false;
+        throwOnExecute = false;
+        Connection connection = dataSource.getConnection();
+        connection.close();
+        connection = dataSource.getConnection();
+        connection.close();
+        assert(connectCount == 1);
+        assert(executeCount == 1);
+
+        // Create 2 connections, free them, and simulate errors when trying to use them.
+        Connection c1 = dataSource.getConnection();  // Use the free connection.
+        Connection c2 = dataSource.getConnection();  // Requres a new connection.
+        assert(executeCount == 2);
+        assert(connectCount == 2);
+        c1.close();
+        c2.close();
+        // There are now 2 connections free for re-use, simulate a network disconnect.
+        throwOnExecute = true;
+        // One connection attempts to be re-used, but it fails and a new one is created.
+        Connection c3 = dataSource.getConnection();
+        assert(executeCount == 2);
+        assert(connectCount == 3);
+        // Restore our network and make sure the 1 remainininig free connect is re-used.
+        throwOnExecute = false;
+        Connection c4 = dataSource.getConnection();
+        assert(executeCount == 3);
+        assert(connectCount == 3);
+        // We are now out of free connections, the next attempt should make a new one.
+        Connection c5 = dataSource.getConnection();
+        assert(executeCount == 3);
+        assert(connectCount == 4);
+    }
+}