Skip to content

Commit e1ad7fc

Browse files
JonathanOadriancole
JonathanO
authored and
adriancole
committed
Remove overriding of retry handler. (OpenFeign#459)
* Remove overriding of retry handler. Setting the retry handler to DEFAULT overrides the retry configuration, making it impossible to specify a default configuration. It seems to be unnecessary since allowing LoadBalancerContext to initialize it in initWithNiwsConfig appears to be sufficient for the case where there is no global configuration too. * Match previous retry behaviour, and add tests.
1 parent 9aa2204 commit e1ad7fc

3 files changed

Lines changed: 112 additions & 2 deletions

File tree

‎ribbon/src/main/java/feign/ribbon/LBClient.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ public static LBClient create(ILoadBalancer lb, IClientConfig clientConfig) {
4848

4949
LBClient(ILoadBalancer lb, IClientConfig clientConfig) {
5050
super(lb, clientConfig);
51-
this.setRetryHandler(RetryHandler.DEFAULT);
5251
this.clientConfig = clientConfig;
5352
connectTimeout = clientConfig.get(CommonClientConfigKey.ConnectTimeout);
5453
readTimeout = clientConfig.get(CommonClientConfigKey.ReadTimeout);

‎ribbon/src/main/java/feign/ribbon/LBClientFactory.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package feign.ribbon;
22

33
import com.netflix.client.ClientFactory;
4+
import com.netflix.client.config.DefaultClientConfigImpl;
45
import com.netflix.client.config.IClientConfig;
56
import com.netflix.loadbalancer.ILoadBalancer;
67

@@ -14,9 +15,16 @@ public interface LBClientFactory {
1415
public static final class Default implements LBClientFactory {
1516
@Override
1617
public LBClient create(String clientName) {
17-
IClientConfig config = ClientFactory.getNamedConfig(clientName);
18+
IClientConfig config = ClientFactory.getNamedConfig(clientName, DisableAutoRetriesByDefaultClientConfig.class);
1819
ILoadBalancer lb = ClientFactory.getNamedLoadBalancer(clientName);
1920
return LBClient.create(lb, config);
2021
}
2122
}
23+
24+
final class DisableAutoRetriesByDefaultClientConfig extends DefaultClientConfigImpl {
25+
@Override
26+
public int getDefaultMaxAutoRetriesNextServer() {
27+
return 0;
28+
}
29+
}
2230
}

‎ribbon/src/test/java/feign/ribbon/RibbonClientTest.java‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@
1919
import static org.hamcrest.core.IsEqual.equalTo;
2020
import static org.junit.Assert.assertEquals;
2121
import static org.junit.Assert.assertThat;
22+
import static org.junit.Assert.assertTrue;
23+
import static org.junit.Assert.fail;
2224

2325
import java.io.IOException;
2426
import java.net.URI;
2527
import java.net.URL;
2628

2729
import org.junit.After;
30+
import org.junit.AfterClass;
31+
import org.junit.BeforeClass;
2832
import org.junit.Rule;
2933
import org.junit.Test;
3034
import org.junit.rules.TestName;
@@ -40,6 +44,8 @@
4044
import feign.Param;
4145
import feign.Request;
4246
import feign.RequestLine;
47+
import feign.RetryableException;
48+
import feign.Retryer;
4349
import feign.client.TrustingSSLSocketFactory;
4450

4551
public class RibbonClientTest {
@@ -51,6 +57,26 @@ public class RibbonClientTest {
5157
@Rule
5258
public final MockWebServer server2 = new MockWebServer();
5359

60+
private static String oldRetryConfig = null;
61+
62+
private static final String SUN_RETRY_PROPERTY = "sun.net.http.retryPost";
63+
64+
@BeforeClass
65+
public static void disableSunRetry() throws Exception {
66+
// The Sun HTTP Client retries all requests once on an IOException, which makes testing retry code harder than would
67+
// be ideal. We can only disable it for post, so lets at least do that.
68+
oldRetryConfig = System.setProperty(SUN_RETRY_PROPERTY, "false");
69+
}
70+
71+
@AfterClass
72+
public static void resetSunRetry() throws Exception {
73+
if (oldRetryConfig == null) {
74+
System.clearProperty(SUN_RETRY_PROPERTY);
75+
} else {
76+
System.setProperty(SUN_RETRY_PROPERTY, oldRetryConfig);
77+
}
78+
}
79+
5480
static String hostAndPort(URL url) {
5581
// our build slaves have underscores in their hostnames which aren't permitted by ribbon
5682
return "localhost:" + url.getPort();
@@ -98,6 +124,83 @@ public void ioExceptionRetry() throws IOException, InterruptedException {
98124
// assertEquals(target.lb().getLoadBalancerStats().getSingleServerStat())
99125
}
100126

127+
@Test
128+
public void ioExceptionFailsAfterTooManyFailures() throws IOException, InterruptedException {
129+
server1.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
130+
server1.enqueue(new MockResponse().setBody("success!"));
131+
132+
getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.url("").url()));
133+
134+
TestInterface
135+
api =
136+
Feign.builder().client(RibbonClient.create()).retryer(Retryer.NEVER_RETRY)
137+
.target(TestInterface.class, "http://" + client());
138+
139+
try {
140+
api.post();
141+
fail("No exception thrown");
142+
} catch (RetryableException ignored) {
143+
144+
}
145+
assertEquals(1, server1.getRequestCount());
146+
// TODO: verify ribbon stats match
147+
// assertEquals(target.lb().getLoadBalancerStats().getSingleServerStat())
148+
}
149+
150+
@Test
151+
public void ribbonRetryConfigurationOnSameServer() throws IOException, InterruptedException {
152+
server1.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
153+
server1.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
154+
server2.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
155+
server2.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
156+
157+
getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.url("").url()) + "," + hostAndPort(server2.url("").url()));
158+
getConfigInstance().setProperty(client() + ".ribbon.MaxAutoRetries", 1);
159+
160+
TestInterface
161+
api =
162+
Feign.builder().client(RibbonClient.create()).retryer(Retryer.NEVER_RETRY)
163+
.target(TestInterface.class, "http://" + client());
164+
165+
try {
166+
api.post();
167+
fail("No exception thrown");
168+
} catch (RetryableException ignored) {
169+
170+
}
171+
assertTrue(server1.getRequestCount() == 2 || server2.getRequestCount() == 2);
172+
assertEquals(2, server1.getRequestCount() + server2.getRequestCount());
173+
// TODO: verify ribbon stats match
174+
// assertEquals(target.lb().getLoadBalancerStats().getSingleServerStat())
175+
}
176+
177+
@Test
178+
public void ribbonRetryConfigurationOnMultipleServers() throws IOException, InterruptedException {
179+
server1.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
180+
server1.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
181+
server2.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
182+
server2.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AT_START));
183+
184+
getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.url("").url()) + "," + hostAndPort(server2.url("").url()));
185+
getConfigInstance().setProperty(client() + ".ribbon.MaxAutoRetriesNextServer", 1);
186+
187+
TestInterface
188+
api =
189+
Feign.builder().client(RibbonClient.create()).retryer(Retryer.NEVER_RETRY)
190+
.target(TestInterface.class, "http://" + client());
191+
192+
try {
193+
api.post();
194+
fail("No exception thrown");
195+
} catch (RetryableException ignored) {
196+
197+
}
198+
assertEquals(1, server1.getRequestCount());
199+
assertEquals(1, server2.getRequestCount());
200+
// TODO: verify ribbon stats match
201+
// assertEquals(target.lb().getLoadBalancerStats().getSingleServerStat())
202+
}
203+
101204
/*
102205
This test-case replicates a bug that occurs when using RibbonRequest with a query string.
103206

0 commit comments

Comments
 (0)