From 99eb1f9b50e8480026146854c710c4aca40d703e Mon Sep 17 00:00:00 2001
From: lutovich <konstantin.lutovich@neotechnology.com>
Date: Mon, 5 Dec 2016 14:15:15 +0100
Subject: [PATCH] Fix off-by-one error in max routing failures

Previously `LoadBalancer` retried routing table lookup one time more than the
configured amount. Relevant setting is `routingFailureLimit`.
---
 .../driver/internal/cluster/LoadBalancer.java |  2 +-
 .../neo4j/driver/internal/EventHandler.java   |  6 +--
 .../internal/cluster/LoadBalancerTest.java    | 49 +++++++++++++++++--
 3 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java
index 17e5406d6f..1a625d351f 100644
--- a/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java
+++ b/driver/src/main/java/org/neo4j/driver/internal/cluster/LoadBalancer.java
@@ -199,7 +199,7 @@ private ClusterComposition lookupRoutingTable() throws InterruptedException, Ser
                     return cluster;
                 }
             }
-            if ( ++failures > settings.maxRoutingFailures )
+            if ( ++failures >= settings.maxRoutingFailures )
             {
                 throw new ServiceUnavailableException( NO_ROUTERS_AVAILABLE );
             }
diff --git a/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java b/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java
index 53d433f3b3..dc76c4e2b7 100644
--- a/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java
+++ b/driver/src/test/java/org/neo4j/driver/internal/EventHandler.java
@@ -18,6 +18,8 @@
  */
 package org.neo4j.driver.internal;
 
+import org.hamcrest.Matcher;
+
 import java.io.PrintStream;
 import java.io.PrintWriter;
 import java.lang.reflect.Constructor;
@@ -32,8 +34,6 @@
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.CopyOnWriteArrayList;
 
-import org.hamcrest.Matcher;
-
 import org.neo4j.driver.internal.util.MatcherFactory;
 import org.neo4j.driver.v1.EventLogger;
 
@@ -110,7 +110,7 @@ public final void assertCount( Matcher<? extends Event> matcher, Matcher<Integer
     {
         synchronized ( events )
         {
-            assertThat( events, (Matcher) count( matcher, count ) );
+            assertThat( "Unexpected count " + count, events, (Matcher) count( matcher, count ) );
         }
     }
 
diff --git a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java
index 2ed4cd4999..b5b83fc82c 100644
--- a/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java
+++ b/driver/src/test/java/org/neo4j/driver/internal/cluster/LoadBalancerTest.java
@@ -18,9 +18,6 @@
  */
 package org.neo4j.driver.internal.cluster;
 
-import java.util.ArrayList;
-import java.util.List;
-
 import org.hamcrest.Matcher;
 import org.junit.Rule;
 import org.junit.Test;
@@ -28,6 +25,9 @@
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import org.neo4j.driver.internal.EventHandler;
 import org.neo4j.driver.internal.net.BoltServerAddress;
 import org.neo4j.driver.internal.spi.Connection;
@@ -40,9 +40,11 @@
 
 import static org.hamcrest.Matchers.any;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 import static org.neo4j.driver.internal.cluster.ClusterTopology.Role.READ;
 import static org.neo4j.driver.internal.cluster.ClusterTopology.Role.ROUTE;
@@ -90,9 +92,15 @@ public void evaluate() throws Throwable
     private final ClusterTopology cluster = new ClusterTopology( events, clock );
 
     private LoadBalancer seedLoadBalancer( String host, int port ) throws Exception
+    {
+        RoutingSettings defaultSettings = new RoutingSettings( MAX_ROUTING_FAILURES, RETRY_TIMEOUT_DELAY );
+        return seedLoadBalancer( host, port, defaultSettings );
+    }
+
+    private LoadBalancer seedLoadBalancer( String host, int port, RoutingSettings settings ) throws Exception
     {
         return new LoadBalancer(
-                new RoutingSettings( MAX_ROUTING_FAILURES, RETRY_TIMEOUT_DELAY ),
+                settings,
                 clock,
                 log,
                 connections,
@@ -351,6 +359,38 @@ public void connectionFailure( BoltServerAddress address )
                         any( ClusterComposition.class ) ) ) );
     }
 
+    @Test
+    public void shouldTryConfiguredMaxRoutingFailures() throws Exception
+    {
+        // given
+        int port = 1337;
+        int maxRoutingFailures = 7;
+        RoutingSettings settings = new RoutingSettings( maxRoutingFailures, 10 );
+
+        coreClusterOn( 20, "one", port, "two" );
+        connections.up( "one", port );
+
+        LoadBalancer routing = seedLoadBalancer( "one", port, settings );
+
+        // when
+        connections.down( "one", port );
+        clock.progress( 25_000 ); // will cause TTL timeout
+
+        try
+        {
+            routing.acquireWriteConnection();
+            fail( "Exception expected" );
+        }
+        catch ( Exception e )
+        {
+            assertThat( e, instanceOf( ServiceUnavailableException.class ) );
+        }
+
+        // then
+        events.assertCount( connectionFailure( "one", port ), equalTo( maxRoutingFailures ) );
+        events.assertCount( connectionFailure( "two", port ), equalTo( maxRoutingFailures ) );
+    }
+
     @Test
     public void shouldFailIfEnoughConnectionAttemptsFail() throws Exception
     {
@@ -570,7 +610,6 @@ public void sleep( long timestamp, long millis )
 
         // then
         assertEquals( new BoltServerAddress( "two", 1337 ), connection.boltServerAddress() );
-        events.printEvents( System.out );
     }
 
     private void coreClusterOn( int ttlSeconds, String leader, int port, String... others )