From 7b0bad4dfef20520225587fe8293d5df577f75eb Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Wed, 10 Oct 2018 15:53:23 +0100 Subject: [PATCH] Fix incorrect round-robin strategy behaviour when there are no available hosts. --- .../strategies/RoundRobinStrategy.java | 6 ++++- .../strategies/RoundRobinStrategyTest.java | 27 ++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategy.java b/components/client/src/main/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategy.java index 45486f68f8..e3c2dc6a26 100644 --- a/components/client/src/main/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategy.java +++ b/components/client/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 choose(Preferences preferences) { ArrayList 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/components/client/src/test/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java index 7e49fc17de..dad6c3e0d6 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/loadbalancing/strategies/RoundRobinStrategyTest.java @@ -15,9 +15,11 @@ */ 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; @@ -25,7 +27,6 @@ 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),