Skip to content

Commit 3705017

Browse files
authored
Allow UrlConnectionHttpClient to retry stream-open IOExceptions and NPEs (#7347)
* Allow UrlConnectionHttpClient to retry stream-open IOExceptions and NPEs * Fix changelog and test * Allow retries when reading the response code fails * Update javadoc and format
1 parent 8b038c7 commit 3705017

4 files changed

Lines changed: 400 additions & 33 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "URL Connection HTTP Client",
4+
"contributor": "",
5+
"description": "Allow retries when the URL Connection HTTP Client encounters an IOException or NullPointerException while accessing request or response body streams."
6+
}

http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.io.UncheckedIOException;
2929
import java.net.HttpURLConnection;
3030
import java.net.InetSocketAddress;
31+
import java.net.ProtocolException;
3132
import java.net.Proxy;
3233
import java.net.URI;
3334
import java.nio.charset.StandardCharsets;
@@ -42,7 +43,6 @@
4243
import java.util.Objects;
4344
import java.util.Optional;
4445
import java.util.function.Consumer;
45-
import java.util.function.Supplier;
4646
import java.util.stream.Collectors;
4747
import javax.net.ssl.HostnameVerifier;
4848
import javax.net.ssl.HttpsURLConnection;
@@ -318,23 +318,23 @@ public HttpExecuteResponse call() throws IOException {
318318

319319
return HttpExecuteResponse.builder()
320320
.response(SdkHttpResponse.builder()
321-
.statusCode(responseCode)
322-
.statusText(connection.getResponseMessage())
323-
// TODO: Don't ignore abort?
324-
.headers(extractHeaders(connection))
325-
.build())
321+
.statusCode(responseCode)
322+
.statusText(connection.getResponseMessage())
323+
// TODO: Don't ignore abort?
324+
.headers(extractHeaders(connection))
325+
.build())
326326
.responseBody(responseBody)
327327
.build();
328328
}
329329

330-
private Optional<OutputStream> tryGetOutputStream() {
331-
return getAndHandle100Bug(() -> invokeSafely(connection::getOutputStream), false);
330+
private Optional<OutputStream> tryGetOutputStream() throws IOException {
331+
return getAndHandle100Bug(connection::getOutputStream, false);
332332
}
333333

334-
private Optional<InputStream> tryGetInputStream() {
334+
private Optional<InputStream> tryGetInputStream() throws IOException {
335335
return responseHasNoContent()
336336
? Optional.empty()
337-
: getAndHandle100Bug(() -> invokeSafely(connection::getInputStream), true);
337+
: getAndHandle100Bug(connection::getInputStream, true);
338338
}
339339

340340
private Optional<InputStream> tryGetErrorStream() {
@@ -361,16 +361,13 @@ private Optional<InputStream> tryGetErrorStream() {
361361
* non-failure cases (2xx, 3xx) or log and return the response without the payload for failure cases (4xx or 5xx)
362362
* .</li>
363363
* </ol>
364+
* <p>
365+
* Convert stream-accessor NPEs to checked {@link IOException}s so the retry policy can evaluate them.
364366
*/
365-
private <T> Optional<T> getAndHandle100Bug(Supplier<T> supplier, boolean failOn100Bug) {
367+
private <T> Optional<T> getAndHandle100Bug(IoSupplier<T> supplier, boolean failOn100Bug) throws IOException {
366368
try {
367369
return Optional.ofNullable(supplier.get());
368-
} catch (RuntimeException e) {
369-
if (e.getCause() instanceof NullPointerException) {
370-
throw new UncheckedIOException(new IOException(
371-
"Unexpected NullPointerException when calling HttpURLConnection", e));
372-
}
373-
370+
} catch (ProtocolException e) {
374371
if (!exceptionCausedBy100HandlingBug(e)) {
375372
throw e;
376373
}
@@ -385,18 +382,44 @@ private <T> Optional<T> getAndHandle100Bug(Supplier<T> supplier, boolean failOn1
385382
return Optional.empty();
386383
}
387384

388-
int responseCode = invokeSafely(connection::getResponseCode);
385+
int responseCode = getResponseCodeSafely(connection);
389386
String message = "Unable to read response payload, because service returned response code "
390387
+ responseCode + " to an Expect: 100-continue request. Using another HTTP client "
391388
+ "implementation (e.g. Apache) removes this limitation.";
392389
throw new UncheckedIOException(new IOException(message, e));
390+
} catch (RuntimeException e) {
391+
if (isNpeOrDirectlyWrapsNpe(e)) {
392+
throw logAndConvertNpe(e);
393+
}
394+
throw e;
393395
}
394396
}
395397

396-
private boolean exceptionCausedBy100HandlingBug(RuntimeException e) {
398+
/**
399+
* Matches the bare and directly wrapped NPE forms emitted by HttpURLConnection stream accessors.
400+
*/
401+
private static boolean isNpeOrDirectlyWrapsNpe(RuntimeException e) {
402+
return e instanceof NullPointerException || e.getCause() instanceof NullPointerException;
403+
}
404+
405+
private IOException logAndConvertNpe(RuntimeException e) {
406+
log.debug(() -> "Converting NPE from HttpURLConnection implementation "
407+
+ connection.getClass().getName() + " to IOException for retry evaluation", e);
408+
return new IOException("Unexpected NullPointerException when calling HttpURLConnection", e);
409+
}
410+
411+
private boolean exceptionCausedBy100HandlingBug(ProtocolException e) {
397412
return requestWasExpect100Continue() &&
398413
e.getMessage() != null &&
399-
e.getMessage().startsWith("java.net.ProtocolException: Server rejected operation");
414+
e.getMessage().startsWith("Server rejected operation");
415+
}
416+
417+
/**
418+
* Supplies a value without converting checked {@link IOException}s to runtime exceptions.
419+
*/
420+
@FunctionalInterface
421+
private interface IoSupplier<T> {
422+
T get() throws IOException;
400423
}
401424

402425
private Boolean requestWasExpect100Continue() {
@@ -406,11 +429,11 @@ private Boolean requestWasExpect100Continue() {
406429
.orElse(false);
407430
}
408431

409-
private boolean responseHasNoContent() {
432+
private boolean responseHasNoContent() throws IOException {
410433
// We cannot account for chunked encoded responses, because we only have access to headers and response code here,
411434
// so we assume chunked encoded responses DO have content.
412435
if (responseHasNoContent == null) {
413-
responseHasNoContent = responseNeverHasPayload(invokeSafely(connection::getResponseCode)) ||
436+
responseHasNoContent = responseNeverHasPayload(getResponseCodeSafely(connection)) ||
414437
Objects.equals(connection.getHeaderField("Content-Length"), "0") ||
415438
Objects.equals(connection.getRequestMethod(), "HEAD");
416439
}
@@ -422,13 +445,9 @@ private boolean responseNeverHasPayload(int responseCode) {
422445
}
423446

424447
/**
425-
* {@link sun.net.www.protocol.http.HttpURLConnection#getInputStream0()} has been observed to intermittently throw
426-
* {@link NullPointerException}s for reasons that still require further investigation, but are assumed to be due to a
427-
* bug in the JDK. Propagating such NPEs is confusing for users and are not subject to being retried on by the default
428-
* retry policy configuration, so instead we bias towards propagating these as {@link IOException}s.
429-
* <p>
430-
* TODO: Determine precise root cause of intermittent NPEs, submit JDK bug report if applicable, and consider applying
431-
* this behavior only on unpatched JVM runtime versions.
448+
* Converts NPEs from {@link HttpURLConnection#getResponseCode()} to checked {@link IOException}s so the retry
449+
* policy can evaluate them. These NPEs can occur when
450+
* {@link HttpURLConnection#disconnect()} races with response access.
432451
*/
433452
private static int getResponseCodeSafely(HttpURLConnection connection) throws IOException {
434453
Validate.paramNotNull(connection, "connection");
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
package software.amazon.awssdk.http.urlconnection;
16+
17+
import static org.assertj.core.api.Assertions.assertThat;
18+
import static software.amazon.awssdk.core.internal.util.ResponseHandlerTestUtils.combinedSyncResponseHandler;
19+
import static utils.HttpTestUtils.executionContext;
20+
import static utils.HttpTestUtils.testClientConfiguration;
21+
22+
import java.io.ByteArrayInputStream;
23+
import java.io.ByteArrayOutputStream;
24+
import java.io.IOException;
25+
import java.io.InputStream;
26+
import java.io.OutputStream;
27+
import java.net.HttpURLConnection;
28+
import java.net.URI;
29+
import java.net.URL;
30+
import java.util.Collections;
31+
import java.util.List;
32+
import java.util.Map;
33+
import java.util.concurrent.atomic.AtomicInteger;
34+
import org.junit.jupiter.api.Test;
35+
import software.amazon.awssdk.core.client.config.SdkClientOption;
36+
import software.amazon.awssdk.core.http.NoopTestRequest;
37+
import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient;
38+
import software.amazon.awssdk.core.internal.http.response.NullErrorResponseHandler;
39+
import software.amazon.awssdk.core.retry.RetryPolicy;
40+
import software.amazon.awssdk.core.retry.backoff.BackoffStrategy;
41+
import software.amazon.awssdk.http.SdkHttpClient;
42+
import software.amazon.awssdk.http.SdkHttpFullRequest;
43+
import software.amazon.awssdk.http.SdkHttpMethod;
44+
45+
public class UrlConnectionHttpClientRetryTest {
46+
@Test
47+
void execute_whenOutputStreamThrowsBareNpe_retriesRequest() {
48+
verifyOutputStreamFailureIsRetried(() -> {
49+
throw new NullPointerException("this.http is null");
50+
});
51+
}
52+
53+
@Test
54+
void execute_whenOutputStreamThrowsWrappedNpe_retriesRequest() {
55+
verifyOutputStreamFailureIsRetried(() -> {
56+
throw new RuntimeException(new NullPointerException("this.http is null"));
57+
});
58+
}
59+
60+
@Test
61+
void execute_whenOutputStreamThrowsIOException_retriesRequest() {
62+
verifyOutputStreamFailureIsRetried(() -> {
63+
throw new IOException("connection closed");
64+
});
65+
}
66+
67+
@Test
68+
void execute_whenInputStreamThrowsBareNpe_retriesRequest() {
69+
AtomicInteger attempts = new AtomicInteger();
70+
SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> new StubHttpURLConnection(toUrl(uri)) {
71+
@Override
72+
public InputStream getInputStream() {
73+
if (attempts.incrementAndGet() == 1) {
74+
throw new NullPointerException("this.http is null");
75+
}
76+
return new ByteArrayInputStream(new byte[0]);
77+
}
78+
79+
// Ensure responseHasNoContent() proceeds to getInputStream().
80+
@Override
81+
public String getHeaderField(String name) {
82+
return null;
83+
}
84+
});
85+
86+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
87+
.uri(URI.create("http://localhost/test"))
88+
.method(SdkHttpMethod.GET)
89+
.build();
90+
verifyFailureIsRetried(transport, request, attempts);
91+
}
92+
93+
@Test
94+
void execute_whenResponseCodeCheckBeforeInputStreamThrowsIOException_retriesRequest() {
95+
AtomicInteger attempts = new AtomicInteger();
96+
AtomicInteger responseCodeCalls = new AtomicInteger();
97+
SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> {
98+
attempts.incrementAndGet();
99+
return new StubHttpURLConnection(toUrl(uri)) {
100+
@Override
101+
public int getResponseCode() throws IOException {
102+
if (responseCodeCalls.incrementAndGet() == 2) {
103+
throw new IOException("connection closed");
104+
}
105+
return HTTP_OK;
106+
}
107+
};
108+
});
109+
110+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
111+
.uri(URI.create("http://localhost/test"))
112+
.method(SdkHttpMethod.GET)
113+
.build();
114+
verifyFailureIsRetried(transport, request, attempts);
115+
}
116+
117+
private void verifyOutputStreamFailureIsRetried(IoRunnable firstAttemptFailure) {
118+
AtomicInteger attempts = new AtomicInteger();
119+
SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> new StubHttpURLConnection(toUrl(uri)) {
120+
@Override
121+
public OutputStream getOutputStream() throws IOException {
122+
if (attempts.incrementAndGet() == 1) {
123+
firstAttemptFailure.run();
124+
}
125+
return super.getOutputStream();
126+
}
127+
});
128+
129+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
130+
.uri(URI.create("http://localhost/test"))
131+
.method(SdkHttpMethod.PUT)
132+
.putHeader("Content-Length", "1")
133+
.contentStreamProvider(() -> new ByteArrayInputStream(new byte[1]))
134+
.build();
135+
verifyFailureIsRetried(transport, request, attempts);
136+
}
137+
138+
private void verifyFailureIsRetried(SdkHttpClient transport,
139+
SdkHttpFullRequest request,
140+
AtomicInteger attempts) {
141+
RetryPolicy retryPolicy = RetryPolicy.builder()
142+
.numRetries(1)
143+
.backoffStrategy(BackoffStrategy.none())
144+
.throttlingBackoffStrategy(BackoffStrategy.none())
145+
.build();
146+
AmazonSyncHttpClient client = new AmazonSyncHttpClient(
147+
testClientConfiguration().toBuilder()
148+
.option(SdkClientOption.SYNC_HTTP_CLIENT, transport)
149+
.option(SdkClientOption.RETRY_POLICY, retryPolicy)
150+
.build());
151+
try {
152+
client.requestExecutionBuilder()
153+
.request(request)
154+
.originalRequest(NoopTestRequest.builder().build())
155+
.executionContext(executionContext(request))
156+
.execute(combinedSyncResponseHandler(null, new NullErrorResponseHandler()));
157+
} finally {
158+
client.close();
159+
}
160+
161+
assertThat(attempts.get()).isEqualTo(2);
162+
}
163+
164+
private static URL toUrl(URI uri) {
165+
try {
166+
return uri.toURL();
167+
} catch (IOException e) {
168+
throw new IllegalArgumentException(e);
169+
}
170+
}
171+
172+
@FunctionalInterface
173+
private interface IoRunnable {
174+
void run() throws IOException;
175+
}
176+
177+
private static class StubHttpURLConnection extends HttpURLConnection {
178+
private StubHttpURLConnection(URL url) {
179+
super(url);
180+
}
181+
182+
@Override
183+
public void connect() {
184+
connected = true;
185+
}
186+
187+
@Override
188+
public void disconnect() {
189+
connected = false;
190+
}
191+
192+
@Override
193+
public boolean usingProxy() {
194+
return false;
195+
}
196+
197+
@Override
198+
public OutputStream getOutputStream() throws IOException {
199+
return new ByteArrayOutputStream();
200+
}
201+
202+
@Override
203+
public InputStream getInputStream() {
204+
return null;
205+
}
206+
207+
@Override
208+
public int getResponseCode() throws IOException {
209+
return HTTP_OK;
210+
}
211+
212+
@Override
213+
public String getResponseMessage() {
214+
return "OK";
215+
}
216+
217+
@Override
218+
public String getHeaderField(String name) {
219+
return "Content-Length".equals(name) ? "0" : null;
220+
}
221+
222+
@Override
223+
public Map<String, List<String>> getHeaderFields() {
224+
return Collections.emptyMap();
225+
}
226+
}
227+
}

0 commit comments

Comments
 (0)