Skip to content

Commit c60a1fc

Browse files
jodastephenjodastephen
authored
Close response in interceptor (#158)
OkHttp has changed to throw an exception if a chain.proceed() is called a second time without closing the first response Also change the retry behaviour to be opt-in
1 parent 4202909 commit c60a1fc

5 files changed

Lines changed: 122 additions & 34 deletions

File tree

‎examples/pom.xml‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<parent>
66
<groupId>com.opengamma</groupId>
77
<artifactId>corporate-parent</artifactId>
8-
<version>2.7.3</version>
8+
<version>2.7.4</version>
99
<relativePath />
1010
</parent>
1111
<groupId>com.opengamma.sdk</groupId>
@@ -15,6 +15,38 @@
1515
<name>SDK-Examples</name>
1616
<description>OpenGamma SDK - Example code to demonstrate usage</description>
1717

18+
<!-- ==================================================================== -->
19+
<build>
20+
<pluginManagement>
21+
<plugins>
22+
<plugin>
23+
<groupId>org.eclipse.m2e</groupId>
24+
<artifactId>lifecycle-mapping</artifactId>
25+
<version>1.0.0</version>
26+
<configuration>
27+
<lifecycleMappingMetadata>
28+
<pluginExecutions>
29+
<pluginExecution>
30+
<pluginExecutionFilter>
31+
<groupId>org.joda</groupId>
32+
<artifactId>joda-beans-maven-plugin</artifactId>
33+
<versionRange>[1.1,)</versionRange>
34+
<goals>
35+
<goal>generate-no-resolve</goal>
36+
</goals>
37+
</pluginExecutionFilter>
38+
<action>
39+
<ignore></ignore>
40+
</action>
41+
</pluginExecution>
42+
</pluginExecutions>
43+
</lifecycleMappingMetadata>
44+
</configuration>
45+
</plugin>
46+
</plugins>
47+
</pluginManagement>
48+
</build>
49+
1850
<!-- ==================================================================== -->
1951
<dependencies>
2052
<!-- OpenGamma, relying on transitive dependencies -->
@@ -39,6 +71,7 @@
3971
<!-- Properties for joda-beans-maven-plugin -->
4072
<!-- Lock in JDK config (not Guava) and stick with older version -->
4173
<joda-beans-maven-plugin.version>1.1</joda-beans-maven-plugin.version>
74+
<joda.beans.version>2.8.0</joda.beans.version>
4275
<joda.beans.config>jdk</joda.beans.config>
4376
<!-- Not installed/deployed -->
4477
<maven.install.skip>true</maven.install.skip>

‎modules/common/src/main/java/com/opengamma/sdk/common/ServiceInvokerBuilder.java‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public final class ServiceInvokerBuilder {
7979
/** The auth client factory. */
8080
private Function<ServiceInvoker, AuthClient> authClientFactory;
8181
/** Times to retry */
82-
private int retries = 1;
82+
private int retries;
8383

8484
//-------------------------------------------------------------------------
8585
/**
@@ -131,6 +131,9 @@ public ServiceInvokerBuilder httpClient(OkHttpClient httpClient) {
131131
* Sets the number of retries for HTTP requests that failed due to system/network issues.
132132
* <p>
133133
* This allows the client to cope with intermittent network failures, such as timeouts.
134+
* <p>
135+
* Retries are primarily handled by the underlying OkHttp library.
136+
* At this level, retries are off by default, and this is the recommended setting.
134137
*
135138
* @param retries how many times to retry
136139
* @return this builder, for method chaining
@@ -217,12 +220,20 @@ public ServiceInvoker build() {
217220
}
218221
// setup HttpClient
219222
TokenInterceptor tokenInterceptor = new TokenInterceptor();
220-
RetryInterceptor retryInterceptor = new RetryInterceptor(retries);
221-
httpClient = httpClient.newBuilder()
222-
.addInterceptor(tokenInterceptor)
223-
.addInterceptor(new UserAgentHeaderInterceptor())
224-
.addInterceptor(retryInterceptor)
225-
.build();
223+
UserAgentHeaderInterceptor userAgentInterceptor = new UserAgentHeaderInterceptor();
224+
if (retries > 0) {
225+
RetryInterceptor retryInterceptor = new RetryInterceptor(retries);
226+
httpClient = httpClient.newBuilder()
227+
.addInterceptor(tokenInterceptor)
228+
.addInterceptor(userAgentInterceptor)
229+
.addInterceptor(retryInterceptor)
230+
.build();
231+
} else {
232+
httpClient = httpClient.newBuilder()
233+
.addInterceptor(tokenInterceptor)
234+
.addInterceptor(userAgentInterceptor)
235+
.build();
236+
}
226237
// setup instance, creating a pure immutable ServiceInvoker, then using it
227238
// care should be taken when altering this code to ensure Java Memory Model semantics are considered
228239
ServiceInvoker invoker = new ServiceInvoker(serviceUrl, httpClient, executorService);
@@ -287,21 +298,19 @@ private RetryInterceptor(int retryCount) {
287298
@Override
288299
public Response intercept(Chain chain) throws IOException {
289300
Request request = chain.request();
290-
Response response = null;
291301
Exception exception = null;
292-
int retries = 0;
293-
while (response == null && retries < retryCount) {
294-
retries++;
302+
for (int retries = 0; retries <= retryCount; retries++) {
295303
try {
296-
response = chain.proceed(request);
297-
} catch (IOException | UncheckedIOException e) {
298-
exception = e;
304+
Response response = chain.proceed(request);
305+
if (response != null) {
306+
return response;
307+
}
308+
} catch (IOException | UncheckedIOException ex) {
309+
exception = ex;
299310
}
300311
}
301-
if (response == null) {
302-
throw new IOException("Failed to perform " + request.method() + " request to given URL after " + retries + " retries: " + request.url().toString(), exception);
303-
}
304-
return response;
312+
throw new IOException("Failed to perform " + request.method() + " request to given URL after " + retryCount +
313+
" retries: " + request.url().toString(), exception);
305314
}
306315
}
307316

@@ -341,6 +350,8 @@ public Response intercept(Chain chain) throws IOException {
341350
if (response.code() != 401) {
342351
return response;
343352
}
353+
// response must be closed before calling chain.proceed() again
354+
response.close();
344355
}
345356

346357
// try to get a new token

‎modules/common/src/test/java/com/opengamma/sdk/common/BasicTest.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public void testAuthGood() {
3939
.authClientFactory(inv -> mockAuth)
4040
.build();
4141
assertThat(invoker.getServiceUrl()).isEqualTo(SERVICE_URL);
42-
assertThat(invoker.getHttpClient().interceptors()).hasSize(4);
42+
assertThat(invoker.getHttpClient().interceptors()).hasSize(3);
4343
assertThat(invoker.getHttpClient().followRedirects()).isTrue();
4444
assertThat(invoker.getExecutor().isShutdown()).isFalse();
4545
invoker.close();
@@ -55,7 +55,7 @@ public void testAuthGoodHttpFactory() {
5555
.authClientFactory(inv -> mockAuth)
5656
.build()) {
5757
assertThat(invoker.getServiceUrl()).isEqualTo(SERVICE_URL);
58-
assertThat(invoker.getHttpClient().interceptors()).hasSize(5); // logging, user-agent & auth& retry plus one from test
58+
assertThat(invoker.getHttpClient().interceptors()).hasSize(4); // logging, user-agent & auth plus one from test
5959
assertThat(invoker.getHttpClient().followRedirects()).isFalse();
6060
}
6161
}
@@ -68,7 +68,7 @@ public void testAuthGoodHttpClient() {
6868
.authClientFactory(inv -> mockAuth)
6969
.build()) {
7070
assertThat(invoker.getServiceUrl()).isEqualTo(SERVICE_URL);
71-
assertThat(invoker.getHttpClient().interceptors()).hasSize(3); // user-agent & auth & retry
71+
assertThat(invoker.getHttpClient().interceptors()).hasSize(2); // user-agent & auth
7272
}
7373
}
7474

@@ -80,7 +80,8 @@ public void testAuthBad() {
8080
.url(serviceInvoker.getServiceUrl().resolve("/test"))
8181
.get()
8282
.build();
83-
assertThatExceptionOfType(AuthenticationException.class).isThrownBy(() -> serviceInvoker.getHttpClient().newCall(testRequest).execute());
83+
assertThatExceptionOfType(AuthenticationException.class)
84+
.isThrownBy(() -> serviceInvoker.getHttpClient().newCall(testRequest).execute());
8485
}
8586
}
8687

‎modules/common/src/test/java/com/opengamma/sdk/common/ServiceInvokerTest.java‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
import com.opengamma.sdk.common.auth.Credentials;
1515

16-
import okhttp3.HttpUrl;
1716
import okhttp3.Request;
1817
import okhttp3.Response;
1918
import okhttp3.mockwebserver.MockResponse;
@@ -30,18 +29,24 @@ public class ServiceInvokerTest {
3029
public void setUp() throws Exception {
3130
server = new MockWebServer();
3231
server.start(18080);
32+
// first call - auth
3333
server.enqueue(new MockResponse().setResponseCode(200).setBody("{\n" +
3434
" \"access_token\": \"testAccessToken\",\n" +
3535
" \"expires_in\": 1,\n" +
3636
" \"token_type\": \"Bearer\"\n" +
3737
"}"));
38+
// first call - valid result
39+
server.enqueue(new MockResponse().setResponseCode(200).setBody("VALID1"));
40+
// second call - token expired
41+
server.enqueue(new MockResponse().setResponseCode(401).setBody(""));
42+
// second call - auth
3843
server.enqueue(new MockResponse().setResponseCode(200).setBody("{\n" +
39-
" \"access_token\": \"<Your access token here>\",\n" +
44+
" \"access_token\": \"testAccessToken\",\n" +
4045
" \"expires_in\": 1,\n" +
4146
" \"token_type\": \"Bearer\"\n" +
4247
"}"));
43-
server.enqueue(new MockResponse().setResponseCode(200).setBody("{}"));
44-
server.enqueue(new MockResponse().setResponseCode(200).setBody("{}"));
48+
// second call - valid result
49+
server.enqueue(new MockResponse().setResponseCode(200).setBody("VALID2"));
4550
}
4651

4752
@AfterEach
@@ -50,20 +55,30 @@ public void tearDown() throws Exception {
5055
}
5156

5257
@Test
53-
public void testNewRequestWithReauthenticationSequence() throws Exception {
54-
58+
public void testAuthFlow() throws Exception {
5559
try (ServiceInvoker invoker = ServiceInvoker.builder(Credentials.ofApiKey("test", "test"))
56-
.serviceUrl(HttpUrl.parse("http://" + server.getHostName() + ":" + server.getPort()))
60+
.serviceUrl(server.url(""))
5761
.build()) {
5862

5963
Request request = new Request.Builder()
6064
.url(invoker.getServiceUrl().resolve("test"))
6165
.get()
6266
.build();
63-
Response response = invoker.getHttpClient().newCall(request).execute();
64-
assertThat(response.code()).isEqualTo(200);
65-
assertThat(response.message()).isEqualTo("OK");
66-
assertThat(response.isSuccessful()).isTrue();
67+
// first call
68+
try (Response response1 = invoker.getHttpClient().newCall(request).execute()) {
69+
assertThat(response1.code()).isEqualTo(200);
70+
assertThat(response1.message()).isEqualTo("OK");
71+
assertThat(response1.body().string()).isEqualTo("VALID1");
72+
assertThat(response1.isSuccessful()).isTrue();
73+
}
74+
// second call
75+
try (Response response2 = invoker.getHttpClient().newCall(request).execute()) {
76+
assertThat(response2.code()).isEqualTo(200);
77+
assertThat(response2.message()).isEqualTo("OK");
78+
assertThat(response2.body().string()).isEqualTo("VALID2");
79+
assertThat(response2.isSuccessful()).isTrue();
80+
}
6781
}
6882
}
83+
6984
}

‎modules/pom.xml‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,34 @@
153153
</executions>
154154
</plugin>
155155
</plugins>
156+
<pluginManagement>
157+
<plugins>
158+
<plugin>
159+
<groupId>org.eclipse.m2e</groupId>
160+
<artifactId>lifecycle-mapping</artifactId>
161+
<version>1.0.0</version>
162+
<configuration>
163+
<lifecycleMappingMetadata>
164+
<pluginExecutions>
165+
<pluginExecution>
166+
<pluginExecutionFilter>
167+
<groupId>org.joda</groupId>
168+
<artifactId>joda-beans-maven-plugin</artifactId>
169+
<versionRange>[1.1,)</versionRange>
170+
<goals>
171+
<goal>generate-no-resolve</goal>
172+
</goals>
173+
</pluginExecutionFilter>
174+
<action>
175+
<ignore></ignore>
176+
</action>
177+
</pluginExecution>
178+
</pluginExecutions>
179+
</lifecycleMappingMetadata>
180+
</configuration>
181+
</plugin>
182+
</plugins>
183+
</pluginManagement>
156184
</build>
157185

158186
<!-- ==================================================================== -->

0 commit comments

Comments
 (0)