Revert "Add idle timeout to the connection pool (JAVA-419)."

olim7t · olim7t · commit 9f3e528ab4af · 2014-12-15T17:26:34.000+01:00
This reverts commits f84cdc1, 38edc2e and f510750. The implementation needs to be reconsidered (see JAVA-573). Due to planning constraints, we are not able to address this in 2.0.9. The feature will be reintroduced in a later version.
diff --git a/driver-core/CHANGELOG.rst b/driver-core/CHANGELOG.rst
@@ -47,7 +47,6 @@ CHANGELOG
 - [improvement] Shuffle the replicas in TokenAwarePolicy.newQueryPlan (JAVA-504)
 - [improvement] Make schema agreement wait tuneable (JAVA-507)
 - [improvement] Document how to inject the driver metrics into another registry (JAVA-494)
-- [improvement] Add idle timeout to the connection pool (JAVA-419)
 - [bug] LatencyAwarePolicy does not shutdown executor on invocation of close (JAVA-516)
 - [improvement] Throw an exception when DCAwareRoundRobinPolicy is built with
   an explicit but null or empty local datacenter (JAVA-451).
diff --git a/driver-core/src/main/java/com/datastax/driver/core/Cluster.java b/driver-core/src/main/java/com/datastax/driver/core/Cluster.java
@@ -1161,8 +1161,6 @@ private Manager(String clusterName, List<InetSocketAddress> contactPoints, Confi
 
             this.metrics = configuration.getMetricsOptions() == null ? null : new Metrics(this);
             this.listeners = new CopyOnWriteArraySet<Host.StateListener>(listeners);
-
-            this.scheduledTasksExecutor.scheduleWithFixedDelay(new TrashIdleConnectionsTask(), 10, 10, TimeUnit.SECONDS);
         }
 
         // Initialization is not too performance intensive and in practice there shouldn't be contention
@@ -2175,19 +2173,6 @@ public void run() {
                 }).start();
             }
         }
-
-        private class TrashIdleConnectionsTask implements Runnable {
-            @Override public void run() {
-                try {
-                    long now = System.currentTimeMillis();
-                    for (SessionManager session : sessions) {
-                        session.trashIdleConnections(now);
-                    }
-                } catch (Exception e) {
-                    logger.warn("Error while trashing idle connections", e);
-                }
-            }
-        }
     }
 
     /**
diff --git a/driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java b/driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java
@@ -265,24 +265,15 @@ public void returnConnection(PooledConnection connection) {
                 close(connection);
         } else {
             if (connections.size() > options().getCoreConnectionsPerHost(hostDistance) && inFlight <= options().getMinSimultaneousRequestsPerConnectionThreshold(hostDistance)) {
-                connection.setTrashTimeIn(options().getIdleTimeoutSeconds());
+                trashConnection(connection);
             } else if (connection.maxAvailableStreams() < MIN_AVAILABLE_STREAMS) {
                 replaceConnection(connection);
             } else {
-                connection.cancelTrashTime();
                 signalAvailableConnection();
             }
         }
     }
 
-    void trashIdleConnections(long now) {
-        for (PooledConnection connection : connections) {
-            if (connection.getTrashTime() < now) {
-                trashConnection(connection);
-            }
-        }
-    }
-
     // Trash the connection and create a new one, but we don't call trashConnection
     // directly because we want to make sure the connection is always trashed.
     private void replaceConnection(PooledConnection connection) {
@@ -299,7 +290,6 @@ private boolean trashConnection(PooledConnection connection) {
                 int opened = open.get();
                 if (opened <= options().getCoreConnectionsPerHost(hostDistance)) {
                     connection.markForTrash.set(false);
-                    connection.cancelTrashTime();
                     return false;
                 }
 
@@ -309,9 +299,13 @@ private boolean trashConnection(PooledConnection connection) {
 
             doTrashConnection(connection);
         }
-        // If compareAndSet failed, it means we raced with another thread that will execute doTrashConnection.
-        // Since trashConnection is called from a scheduled task, we're sure that the current thread did not modify
-        // inFlight, so the other thread will take care of closing the connection if necessary (i.e. if inFlight == 0).
+        // If compareAndSet failed, it means we raced and another thread will execute doTrashConnection.
+        // If the connection needs to be closed (inFlight == 0), we don't need to do it here because the other thread will necessarily do
+        // it:
+        // - the current thread decremented inFlight in returnConnection
+        // - we know it did it before the connection was trashed, because otherwise it would have entered `if (trash.contains(connection))`
+        //   in returnConnection and not arrived here.
+        // - so the other thread will see the up-to-date value of inFlight and take appropriate action.
         return true;
     }
 
diff --git a/driver-core/src/main/java/com/datastax/driver/core/PooledConnection.java b/driver-core/src/main/java/com/datastax/driver/core/PooledConnection.java
@@ -25,9 +25,6 @@ class PooledConnection extends Connection {
 
     private final HostConnectionPool pool;
 
-    /** The instant when the connection should be trashed after being idle for too long */
-    private volatile long trashTime = Long.MAX_VALUE;
-
     /** Used in {@link HostConnectionPool} to handle races between two threads trying to trash the same connection */
     final AtomicBoolean markForTrash = new AtomicBoolean();
 
@@ -63,16 +60,4 @@ protected void notifyOwnerWhenDefunct(boolean hostIsDown) {
             pool.replaceDefunctConnection(this);
         }
     }
-
-    long getTrashTime() {
-        return trashTime;
-    }
-
-    void cancelTrashTime() {
-        trashTime = Long.MAX_VALUE;
-    }
-
-    void setTrashTimeIn(int timeoutSeconds) {
-        trashTime = System.currentTimeMillis() + 1000 * timeoutSeconds;
-    }
 }
diff --git a/driver-core/src/main/java/com/datastax/driver/core/PoolingOptions.java b/driver-core/src/main/java/com/datastax/driver/core/PoolingOptions.java
@@ -51,7 +51,6 @@ public class PoolingOptions {
     private static final int DEFAULT_MAX_POOL_LOCAL = 8;
     private static final int DEFAULT_MAX_POOL_REMOTE = 2;
 
-    private static final int DEFAULT_IDLE_TIMEOUT_SECONDS = 120;
     private static final int DEFAULT_POOL_TIMEOUT_MILLIS = 5000;
 
     private volatile Cluster.Manager manager;
@@ -62,7 +61,6 @@ public class PoolingOptions {
     private final int[] coreConnections = new int[] { DEFAULT_CORE_POOL_LOCAL, DEFAULT_CORE_POOL_REMOTE, 0 };
     private final int[] maxConnections = new int[] { DEFAULT_MAX_POOL_LOCAL , DEFAULT_MAX_POOL_REMOTE, 0 };
 
-    private volatile int idleTimeoutSeconds = DEFAULT_IDLE_TIMEOUT_SECONDS;
     private volatile int poolTimeoutMillis = DEFAULT_POOL_TIMEOUT_MILLIS;
 
     public PoolingOptions() {}
@@ -223,33 +221,6 @@ public synchronized PoolingOptions setMaxConnectionsPerHost(HostDistance distanc
         return this;
     }
 
-    /**
-     * Returns the timeout before an idle connection is removed.
-     *
-     * @return the timeout.
-     */
-    public int getIdleTimeoutSeconds() {
-        return idleTimeoutSeconds;
-    }
-
-    /**
-     * Sets the timeout before an idle connection is removed.
-     * <p>
-     * The order of magnitude should be a few minutes (the default is 120 seconds). The
-     * timeout that triggers the removal has a granularity of 10 seconds.
-     *
-     * @param idleTimeoutSeconds the new timeout in seconds.
-     * @return this {@code PoolingOptions}.
-     *
-     * @throws IllegalArgumentException if the timeout is negative.
-     */
-    public PoolingOptions setIdleTimeoutSeconds(int idleTimeoutSeconds) {
-        if (idleTimeoutSeconds < 0)
-            throw new IllegalArgumentException("Idle timeout must be positive");
-        this.idleTimeoutSeconds = idleTimeoutSeconds;
-        return this;
-    }
-
     /**
      * Returns the timeout when trying to acquire a connection from a host's pool.
      *
diff --git a/driver-core/src/main/java/com/datastax/driver/core/SessionManager.java b/driver-core/src/main/java/com/datastax/driver/core/SessionManager.java
@@ -15,6 +15,7 @@
  */
 package com.datastax.driver.core;
 
+import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.nio.ByteBuffer;
 import java.util.*;
@@ -28,6 +29,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+
+import com.datastax.driver.core.exceptions.AuthenticationException;
 import com.datastax.driver.core.exceptions.DriverInternalError;
 import com.datastax.driver.core.exceptions.UnsupportedFeatureException;
 import com.datastax.driver.core.policies.LoadBalancingPolicy;
@@ -540,12 +543,6 @@ ResultSetFuture executeQuery(Message.Request msg, Statement statement) {
         return future;
     }
 
-    void trashIdleConnections(long now) {
-        for (HostConnectionPool pool : pools.values()) {
-            pool.trashIdleConnections(now);
-        }
-    }
-
     private static class State implements Session.State {
 
         private final SessionManager session;
diff --git a/driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTimeoutTest.java b/driver-core/src/test/java/com/datastax/driver/core/HostConnectionPoolTimeoutTest.java
