Fix incorrect round-robin strategy behaviour when there are no availa…

…ble hosts. (ExpediaGroup#307)
mikkokar · Oct 10, 2018 · 2271ed1 · 2271ed1
1 parent ab09785
commit 2271ed1
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 7 deletions.
diff --git a/...ent/src/main/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategy.java b/...ent/src/main/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategy.java
@@ -69,7 +69,11 @@ public LoadBalancer create(Environment environment, Configuration strategyConfig
     @Override
     public Optional<RemoteHost> choose(Preferences preferences) {
         ArrayList<RemoteHost> remoteHosts = origins.get();
-        return Optional.ofNullable(remoteHosts.get(index.getAndIncrement() % remoteHosts.size()));
+        if (remoteHosts == null || remoteHosts.isEmpty()) {
+            return Optional.empty();
+        } else {
+            return Optional.ofNullable(remoteHosts.get(index.getAndIncrement() % remoteHosts.size()));
+        }
     }
 
     @Override

diff --git a/...est/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java b/...est/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java
@@ -15,17 +15,18 @@
  */
 package com.hotels.styx.client.loadbalancing.strategies;
 
+import com.google.common.collect.ImmutableList;
 import com.hotels.styx.api.Environment;
 import com.hotels.styx.api.HttpHandler;
 import com.hotels.styx.api.Id;
+import com.hotels.styx.api.configuration.Configuration;
 import com.hotels.styx.api.extension.ActiveOrigins;
 import com.hotels.styx.api.extension.Origin;
 import com.hotels.styx.api.extension.OriginsSnapshot;
 import com.hotels.styx.api.extension.RemoteHost;
 import com.hotels.styx.api.extension.loadbalancing.spi.LoadBalancer;
 import com.hotels.styx.api.extension.loadbalancing.spi.LoadBalancingMetric;
 import com.hotels.styx.api.extension.loadbalancing.spi.LoadBalancingMetricSupplier;
-import com.hotels.styx.api.configuration.Configuration;
 import com.hotels.styx.client.connectionpool.stubs.StubConnectionFactory;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
@@ -50,6 +51,8 @@ public class RoundRobinStrategyTest {
     private static final RemoteHost HOST_5 = remoteHostFor("localhost", 5);
     private static final RemoteHost HOST_6 = remoteHostFor("localhost", 6);
     private Id APP_ID = id("app");
+    private Environment environment;
+    private Configuration configuration;
 
     private static RemoteHost remoteHostFor(String host, int port) {
         Origin origin = newOriginBuilder(host, port).build();
@@ -61,13 +64,15 @@ private static RemoteHost remoteHostFor(String host, int port) {
     }
 
     private LoadBalancer strategy;
-    private ActiveOrigins activeOriginsMock;
 
     @BeforeMethod
     public void setUp() {
-        activeOriginsMock = mock(ActiveOrigins.class);
-        when(activeOriginsMock.snapshot()).thenReturn(asList(HOST_1, HOST_2, HOST_3));
-        strategy = new RoundRobinStrategy.Factory().create(mock(Environment.class), mock(Configuration.class), activeOriginsMock);
+        environment = mock(Environment.class);
+        configuration = mock(Configuration.class);
+        strategy = new RoundRobinStrategy.Factory().create(
+                environment,
+                configuration,
+                () -> asList(HOST_1, HOST_2, HOST_3));
     }
 
     @Test
@@ -79,15 +84,25 @@ public void cyclesThroughOrigins() {
         assertThat(strategy.choose(null), is(Optional.of(HOST_2)));
     }
 
+    @Test
+    public void returnsEmptyIfNoActiveOrigins() {
+        strategy = new RoundRobinStrategy.Factory().create(environment, configuration, ImmutableList::of);
+        assertThat(strategy.choose(null), is(Optional.empty()));
+    }
+
     @Test
     public void refreshesAtOriginsChange() {
+        ActiveOrigins activeOrigins = mock(ActiveOrigins.class);
+        when(activeOrigins.snapshot()).thenReturn(asList(HOST_1, HOST_2, HOST_3));
+        strategy = new RoundRobinStrategy.Factory().create(environment, configuration, activeOrigins);
+
         assertThat(strategy.choose(null), is(Optional.of(HOST_1)));
         assertThat(strategy.choose(null), is(Optional.of(HOST_2)));
         assertThat(strategy.choose(null), is(Optional.of(HOST_3)));
         assertThat(strategy.choose(null), is(Optional.of(HOST_1)));
         assertThat(strategy.choose(null), is(Optional.of(HOST_2)));
 
-        when(activeOriginsMock.snapshot()).thenReturn(asList(HOST_2, HOST_4, HOST_6));
+        when(activeOrigins.snapshot()).thenReturn(asList(HOST_2, HOST_4, HOST_6));
         strategy.originsChanged(new OriginsSnapshot(
                 APP_ID,
                 asList(HOST_2, HOST_4, HOST_6),