Removed the invalid parameterized tag in SocketClient as the test only tests on a mocked channel

Fixed the bug where the channel might not be closed if socketClient.start() throws exceptions

Added tests to verify channel is closed after construction failure and IO failures for both plaintext and encrypted channel

Author: Zhen

driver/src/main/java/org/neo4j/driver/internal/cluster/ClusterRoutingTable.java

@@ -20,6 +20,7 @@
 package org.neo4j.driver.internal.cluster;
 
 import java.util.HashSet;
+import java.util.Set;
 
 import org.neo4j.driver.internal.net.BoltServerAddress;
 import org.neo4j.driver.internal.util.Clock;
@@ -33,7 +34,9 @@ public class ClusterRoutingTable implements RoutingTable
 
     private final Clock clock;
     private long expirationTimeout;
-    private final RoundRobinAddressSet readers, writers, routers;
+    private final RoundRobinAddressSet readers;
+    private final RoundRobinAddressSet writers;
+    private final RoundRobinAddressSet routers;
 
     public ClusterRoutingTable( Clock clock, BoltServerAddress... routingAddresses )
     {
@@ -52,7 +55,7 @@ private ClusterRoutingTable( Clock clock )
     }
 
     @Override
-    public boolean stale()
+    public boolean isStale()
     {
         return expirationTimeout < clock.millis() || // the expiration timeout has been reached
                routers.size() <= MIN_ROUTERS || // we need to discover more routing servers
@@ -61,7 +64,7 @@ public boolean stale()
     }
 
     @Override
-    public synchronized HashSet<BoltServerAddress> update( ClusterComposition cluster )
+    public synchronized Set<BoltServerAddress> update( ClusterComposition cluster )
     {
         expirationTimeout = cluster.expirationTimestamp;
         HashSet<BoltServerAddress> removed = new HashSet<>();

driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java

@@ -18,7 +18,7 @@
  */
 package org.neo4j.driver.internal.cluster;
 
-import java.util.HashSet;
+import java.util.Set;
 
 import org.neo4j.driver.internal.RoutingErrorHandler;
 import org.neo4j.driver.internal.net.BoltServerAddress;
@@ -29,19 +29,14 @@
 import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
 
 import static java.lang.String.format;
-import static org.neo4j.driver.internal.cluster.Rediscovery.lookupRoutingTable;
 
 public final class LoadBalancer implements RoutingErrorHandler, AutoCloseable
 {
-    private final RoutingSettings settings;
-    private final Clock clock;
-    // dependencies
     private final Logger log;
+
     private final ConnectionPool connections;
-    // state
-    private final ClusterComposition.Provider provider;
     private final RoutingTable routingTable;
-
+    private final Rediscovery rediscovery;
 
     public LoadBalancer(
             RoutingSettings settings,
@@ -54,20 +49,24 @@ public LoadBalancer(
                 new ClusterComposition.Provider.Default( clock, log ) );
     }
 
-    LoadBalancer(
+    private LoadBalancer(
             RoutingSettings settings,
             Clock clock,
             Logger log,
             ConnectionPool connections,
             RoutingTable routingTable,
             ClusterComposition.Provider provider ) throws ServiceUnavailableException
     {
-        this.settings = settings;
-        this.clock = clock;
+        this( log, connections, routingTable, new Rediscovery( settings, clock, log, provider ) );
+    }
+
+    LoadBalancer( Logger log, ConnectionPool connections, RoutingTable routingTable, Rediscovery rediscovery )
+            throws ServiceUnavailableException
+    {
         this.log = log;
         this.connections = connections;
         this.routingTable = routingTable;
-        this.provider = provider;
+        this.rediscovery = rediscovery;
 
         // initialize the routing table
         ensureRouting();
@@ -135,15 +134,14 @@ private synchronized void forget( BoltServerAddress address )
 
     private synchronized void ensureRouting() throws ServiceUnavailableException
     {
-        if ( routingTable.stale() )
+        if ( routingTable.isStale() )
         {
             log.info( "Routing information is stale. %s", routingTable );
             try
             {
                 // get a new routing table
-                ClusterComposition cluster = lookupRoutingTable( settings, clock, log,
-                        connections, routingTable, provider );
-                HashSet<BoltServerAddress> removed = routingTable.update( cluster );
+                ClusterComposition cluster = rediscovery.lookupRoutingTable( connections, routingTable );
+                Set<BoltServerAddress> removed = routingTable.update( cluster );
                 // purge connections to removed addresses
                 for ( BoltServerAddress address : removed )
                 {

driver/src/main/java/org/neo4j/driver/internal/cluster/Rediscovery.java

@@ -31,12 +31,22 @@ public class Rediscovery
 {
     private static final String NO_ROUTERS_AVAILABLE = "Could not perform discovery. No routing servers available.";
 
-    // Given a old routing table and connection pool, use the connection composition provider to obtain a new
+    private final RoutingSettings settings;
+    private final Clock clock;
+    private final Logger logger;
+    private final ClusterComposition.Provider provider;
+
+    public Rediscovery( RoutingSettings settings, Clock clock, Logger logger, ClusterComposition.Provider provider )
+    {
+        this.settings = settings;
+        this.clock = clock;
+        this.logger = logger;
+        this.provider = provider;
+    }
+
+    // Given the current routing table and connection pool, use the connection composition provider to fetch a new
     // cluster composition, which would be used to update the routing table and connection pool
-    public static ClusterComposition lookupRoutingTable(
-            RoutingSettings settings, Clock clock, Logger log,
-            ConnectionPool connections, RoutingTable routingTable,
-            ClusterComposition.Provider provider )
+    public ClusterComposition lookupRoutingTable( ConnectionPool connections, RoutingTable routingTable )
             throws InterruptedException, ServiceUnavailableException
     {
         int size = routingTable.routerSize(), failures = 0;
@@ -63,17 +73,16 @@ public static ClusterComposition lookupRoutingTable(
                 try ( Connection connection = connections.acquire( address ) )
                 {
                     cluster = provider.getClusterComposition( connection );
-                    log.info( "Got cluster composition %s", cluster );
+                    logger.info( "Got cluster composition %s", cluster );
                 }
                 catch ( Exception e )
                 {
-                    log.error( format( "Failed to connect to routing server '%s'.", address ), e );
+                    logger.error( format( "Failed to connect to routing server '%s'.", address ), e );
                     continue;
                 }
                 if ( cluster == null || !cluster.isValid() )
                 {
-                    log.info(
-                            "Server <%s> unable to perform routing capability, dropping from list of routers.",
+                    logger.info( "Server <%s> unable to perform routing capability, dropping from list of routers.",
                             address );
                     routingTable.removeRouter( address );
                     if ( --size == 0 )

driver/src/main/java/org/neo4j/driver/internal/cluster/RoutingTable.java

@@ -18,15 +18,15 @@
  */
 package org.neo4j.driver.internal.cluster;
 
-import java.util.HashSet;
+import java.util.Set;
 
 import org.neo4j.driver.internal.net.BoltServerAddress;
 
 public interface RoutingTable
 {
-    boolean stale();
+    boolean isStale();
 
-    HashSet<BoltServerAddress> update( ClusterComposition cluster );
+    Set<BoltServerAddress> update( ClusterComposition cluster );
 
     void forget( BoltServerAddress address );
 

driver/src/main/java/org/neo4j/driver/internal/net/SocketClient.java

@@ -203,7 +203,7 @@ public void stop()
             }
             else
             {
-                throw new ClientException( "Unable to close socket connection properly." + e.getMessage(), e );
+                logger.warn( "Unable to close socket connection properly: '" + e.getMessage() + "'", e );
             }
         }
     }

driver/src/main/java/org/neo4j/driver/internal/net/SocketConnection.java

@@ -60,10 +60,13 @@ public class SocketConnection implements Connection
 
     public SocketConnection( BoltServerAddress address, SecurityPlan securityPlan, int timeoutMillis, Logging logging )
     {
-        this.logger = logging.getLog( format( "conn-%s", UUID.randomUUID().toString() ) );
-        this.socket = new SocketClient( address, securityPlan, timeoutMillis, logger );
-        this.responseHandler = createResponseHandler( logger );
-        this.socket.start();
+        this( address, securityPlan, timeoutMillis,
+                logging.getLog( format( "conn-%s", UUID.randomUUID().toString() ) ) );
+    }
+
+    private SocketConnection( BoltServerAddress address, SecurityPlan securityPlan, int timeoutMillis, Logger logger )
+    {
+        this( new SocketClient( address, securityPlan, timeoutMillis, logger ), null, logger );
     }
 
     /**
@@ -81,7 +84,16 @@ public SocketConnection( SocketClient socket, InternalServerInfo serverInfo, Log
         this.serverInfo = serverInfo;
         this.logger = logger;
         this.responseHandler = createResponseHandler( logger );
-        this.socket.start();
+
+        try
+        {
+            this.socket.start();
+        }
+        catch ( Throwable e )
+        {
+            close();
+            throw e;
+        }
     }
 
     private SocketResponseHandler createResponseHandler( Logger logger )

driver/src/main/java/org/neo4j/driver/internal/net/SocketConnector.java

@@ -47,8 +47,7 @@ public SocketConnector( ConnectionSettings connectionSettings, SecurityPlan secu
     @Override
     public final Connection connect( BoltServerAddress address )
     {
-        Connection connection = createConnection( address, securityPlan, connectionSettings.timeoutMillis(),
-                logging );
+        Connection connection = createConnection( address, securityPlan, connectionSettings.timeoutMillis(), logging );
 
         // Because SocketConnection is not thread safe, wrap it in this guard
         // to ensure concurrent access leads causes application errors