diff --git a/.changes/next-release/bugfix-UrlConnectionClient-8b41f2c.json b/.changes/next-release/bugfix-UrlConnectionClient-8b41f2c.json new file mode 100644 index 000000000000..729e4b872925 --- /dev/null +++ b/.changes/next-release/bugfix-UrlConnectionClient-8b41f2c.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "URL Connection HTTP Client", + "contributor": "", + "description": "Allow retries when the URL Connection HTTP Client encounters an IOException or NullPointerException while accessing request or response body streams." +} diff --git a/http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java b/http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java index 6537ec24753a..87009f29d999 100644 --- a/http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java +++ b/http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java @@ -28,6 +28,7 @@ import java.io.UncheckedIOException; import java.net.HttpURLConnection; import java.net.InetSocketAddress; +import java.net.ProtocolException; import java.net.Proxy; import java.net.URI; import java.nio.charset.StandardCharsets; @@ -42,7 +43,6 @@ import java.util.Objects; import java.util.Optional; import java.util.function.Consumer; -import java.util.function.Supplier; import java.util.stream.Collectors; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; @@ -318,23 +318,23 @@ public HttpExecuteResponse call() throws IOException { return HttpExecuteResponse.builder() .response(SdkHttpResponse.builder() - .statusCode(responseCode) - .statusText(connection.getResponseMessage()) - // TODO: Don't ignore abort? - .headers(extractHeaders(connection)) - .build()) + .statusCode(responseCode) + .statusText(connection.getResponseMessage()) + // TODO: Don't ignore abort? + .headers(extractHeaders(connection)) + .build()) .responseBody(responseBody) .build(); } - private Optional tryGetOutputStream() { - return getAndHandle100Bug(() -> invokeSafely(connection::getOutputStream), false); + private Optional tryGetOutputStream() throws IOException { + return getAndHandle100Bug(connection::getOutputStream, false); } - private Optional tryGetInputStream() { + private Optional tryGetInputStream() throws IOException { return responseHasNoContent() ? Optional.empty() - : getAndHandle100Bug(() -> invokeSafely(connection::getInputStream), true); + : getAndHandle100Bug(connection::getInputStream, true); } private Optional tryGetErrorStream() { @@ -361,16 +361,13 @@ private Optional tryGetErrorStream() { * non-failure cases (2xx, 3xx) or log and return the response without the payload for failure cases (4xx or 5xx) * . * + *

+ * Convert stream-accessor NPEs to checked {@link IOException}s so the retry policy can evaluate them. */ - private Optional getAndHandle100Bug(Supplier supplier, boolean failOn100Bug) { + private Optional getAndHandle100Bug(IoSupplier supplier, boolean failOn100Bug) throws IOException { try { return Optional.ofNullable(supplier.get()); - } catch (RuntimeException e) { - if (e.getCause() instanceof NullPointerException) { - throw new UncheckedIOException(new IOException( - "Unexpected NullPointerException when calling HttpURLConnection", e)); - } - + } catch (ProtocolException e) { if (!exceptionCausedBy100HandlingBug(e)) { throw e; } @@ -385,18 +382,44 @@ private Optional getAndHandle100Bug(Supplier supplier, boolean failOn1 return Optional.empty(); } - int responseCode = invokeSafely(connection::getResponseCode); + int responseCode = getResponseCodeSafely(connection); String message = "Unable to read response payload, because service returned response code " + responseCode + " to an Expect: 100-continue request. Using another HTTP client " + "implementation (e.g. Apache) removes this limitation."; throw new UncheckedIOException(new IOException(message, e)); + } catch (RuntimeException e) { + if (isNpeOrDirectlyWrapsNpe(e)) { + throw logAndConvertNpe(e); + } + throw e; } } - private boolean exceptionCausedBy100HandlingBug(RuntimeException e) { + /** + * Matches the bare and directly wrapped NPE forms emitted by HttpURLConnection stream accessors. + */ + private static boolean isNpeOrDirectlyWrapsNpe(RuntimeException e) { + return e instanceof NullPointerException || e.getCause() instanceof NullPointerException; + } + + private IOException logAndConvertNpe(RuntimeException e) { + log.debug(() -> "Converting NPE from HttpURLConnection implementation " + + connection.getClass().getName() + " to IOException for retry evaluation", e); + return new IOException("Unexpected NullPointerException when calling HttpURLConnection", e); + } + + private boolean exceptionCausedBy100HandlingBug(ProtocolException e) { return requestWasExpect100Continue() && e.getMessage() != null && - e.getMessage().startsWith("java.net.ProtocolException: Server rejected operation"); + e.getMessage().startsWith("Server rejected operation"); + } + + /** + * Supplies a value without converting checked {@link IOException}s to runtime exceptions. + */ + @FunctionalInterface + private interface IoSupplier { + T get() throws IOException; } private Boolean requestWasExpect100Continue() { @@ -406,11 +429,11 @@ private Boolean requestWasExpect100Continue() { .orElse(false); } - private boolean responseHasNoContent() { + private boolean responseHasNoContent() throws IOException { // We cannot account for chunked encoded responses, because we only have access to headers and response code here, // so we assume chunked encoded responses DO have content. if (responseHasNoContent == null) { - responseHasNoContent = responseNeverHasPayload(invokeSafely(connection::getResponseCode)) || + responseHasNoContent = responseNeverHasPayload(getResponseCodeSafely(connection)) || Objects.equals(connection.getHeaderField("Content-Length"), "0") || Objects.equals(connection.getRequestMethod(), "HEAD"); } @@ -422,13 +445,9 @@ private boolean responseNeverHasPayload(int responseCode) { } /** - * {@link sun.net.www.protocol.http.HttpURLConnection#getInputStream0()} has been observed to intermittently throw - * {@link NullPointerException}s for reasons that still require further investigation, but are assumed to be due to a - * bug in the JDK. Propagating such NPEs is confusing for users and are not subject to being retried on by the default - * retry policy configuration, so instead we bias towards propagating these as {@link IOException}s. - *

- * TODO: Determine precise root cause of intermittent NPEs, submit JDK bug report if applicable, and consider applying - * this behavior only on unpatched JVM runtime versions. + * Converts NPEs from {@link HttpURLConnection#getResponseCode()} to checked {@link IOException}s so the retry + * policy can evaluate them. These NPEs can occur when + * {@link HttpURLConnection#disconnect()} races with response access. */ private static int getResponseCodeSafely(HttpURLConnection connection) throws IOException { Validate.paramNotNull(connection, "connection"); diff --git a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientRetryTest.java b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientRetryTest.java new file mode 100644 index 000000000000..5483a4dd22cc --- /dev/null +++ b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientRetryTest.java @@ -0,0 +1,227 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ +package software.amazon.awssdk.http.urlconnection; + +import static org.assertj.core.api.Assertions.assertThat; +import static software.amazon.awssdk.core.internal.util.ResponseHandlerTestUtils.combinedSyncResponseHandler; +import static utils.HttpTestUtils.executionContext; +import static utils.HttpTestUtils.testClientConfiguration; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.HttpURLConnection; +import java.net.URI; +import java.net.URL; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.core.client.config.SdkClientOption; +import software.amazon.awssdk.core.http.NoopTestRequest; +import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient; +import software.amazon.awssdk.core.internal.http.response.NullErrorResponseHandler; +import software.amazon.awssdk.core.retry.RetryPolicy; +import software.amazon.awssdk.core.retry.backoff.BackoffStrategy; +import software.amazon.awssdk.http.SdkHttpClient; +import software.amazon.awssdk.http.SdkHttpFullRequest; +import software.amazon.awssdk.http.SdkHttpMethod; + +public class UrlConnectionHttpClientRetryTest { + @Test + void execute_whenOutputStreamThrowsBareNpe_retriesRequest() { + verifyOutputStreamFailureIsRetried(() -> { + throw new NullPointerException("this.http is null"); + }); + } + + @Test + void execute_whenOutputStreamThrowsWrappedNpe_retriesRequest() { + verifyOutputStreamFailureIsRetried(() -> { + throw new RuntimeException(new NullPointerException("this.http is null")); + }); + } + + @Test + void execute_whenOutputStreamThrowsIOException_retriesRequest() { + verifyOutputStreamFailureIsRetried(() -> { + throw new IOException("connection closed"); + }); + } + + @Test + void execute_whenInputStreamThrowsBareNpe_retriesRequest() { + AtomicInteger attempts = new AtomicInteger(); + SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> new StubHttpURLConnection(toUrl(uri)) { + @Override + public InputStream getInputStream() { + if (attempts.incrementAndGet() == 1) { + throw new NullPointerException("this.http is null"); + } + return new ByteArrayInputStream(new byte[0]); + } + + // Ensure responseHasNoContent() proceeds to getInputStream(). + @Override + public String getHeaderField(String name) { + return null; + } + }); + + SdkHttpFullRequest request = SdkHttpFullRequest.builder() + .uri(URI.create("http://localhost/test")) + .method(SdkHttpMethod.GET) + .build(); + verifyFailureIsRetried(transport, request, attempts); + } + + @Test + void execute_whenResponseCodeCheckBeforeInputStreamThrowsIOException_retriesRequest() { + AtomicInteger attempts = new AtomicInteger(); + AtomicInteger responseCodeCalls = new AtomicInteger(); + SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> { + attempts.incrementAndGet(); + return new StubHttpURLConnection(toUrl(uri)) { + @Override + public int getResponseCode() throws IOException { + if (responseCodeCalls.incrementAndGet() == 2) { + throw new IOException("connection closed"); + } + return HTTP_OK; + } + }; + }); + + SdkHttpFullRequest request = SdkHttpFullRequest.builder() + .uri(URI.create("http://localhost/test")) + .method(SdkHttpMethod.GET) + .build(); + verifyFailureIsRetried(transport, request, attempts); + } + + private void verifyOutputStreamFailureIsRetried(IoRunnable firstAttemptFailure) { + AtomicInteger attempts = new AtomicInteger(); + SdkHttpClient transport = UrlConnectionHttpClient.create(uri -> new StubHttpURLConnection(toUrl(uri)) { + @Override + public OutputStream getOutputStream() throws IOException { + if (attempts.incrementAndGet() == 1) { + firstAttemptFailure.run(); + } + return super.getOutputStream(); + } + }); + + SdkHttpFullRequest request = SdkHttpFullRequest.builder() + .uri(URI.create("http://localhost/test")) + .method(SdkHttpMethod.PUT) + .putHeader("Content-Length", "1") + .contentStreamProvider(() -> new ByteArrayInputStream(new byte[1])) + .build(); + verifyFailureIsRetried(transport, request, attempts); + } + + private void verifyFailureIsRetried(SdkHttpClient transport, + SdkHttpFullRequest request, + AtomicInteger attempts) { + RetryPolicy retryPolicy = RetryPolicy.builder() + .numRetries(1) + .backoffStrategy(BackoffStrategy.none()) + .throttlingBackoffStrategy(BackoffStrategy.none()) + .build(); + AmazonSyncHttpClient client = new AmazonSyncHttpClient( + testClientConfiguration().toBuilder() + .option(SdkClientOption.SYNC_HTTP_CLIENT, transport) + .option(SdkClientOption.RETRY_POLICY, retryPolicy) + .build()); + try { + client.requestExecutionBuilder() + .request(request) + .originalRequest(NoopTestRequest.builder().build()) + .executionContext(executionContext(request)) + .execute(combinedSyncResponseHandler(null, new NullErrorResponseHandler())); + } finally { + client.close(); + } + + assertThat(attempts.get()).isEqualTo(2); + } + + private static URL toUrl(URI uri) { + try { + return uri.toURL(); + } catch (IOException e) { + throw new IllegalArgumentException(e); + } + } + + @FunctionalInterface + private interface IoRunnable { + void run() throws IOException; + } + + private static class StubHttpURLConnection extends HttpURLConnection { + private StubHttpURLConnection(URL url) { + super(url); + } + + @Override + public void connect() { + connected = true; + } + + @Override + public void disconnect() { + connected = false; + } + + @Override + public boolean usingProxy() { + return false; + } + + @Override + public OutputStream getOutputStream() throws IOException { + return new ByteArrayOutputStream(); + } + + @Override + public InputStream getInputStream() { + return null; + } + + @Override + public int getResponseCode() throws IOException { + return HTTP_OK; + } + + @Override + public String getResponseMessage() { + return "OK"; + } + + @Override + public String getHeaderField(String name) { + return "Content-Length".equals(name) ? "0" : null; + } + + @Override + public Map> getHeaderFields() { + return Collections.emptyMap(); + } + } +} diff --git a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java index 55b4410c6f36..d658370821f5 100644 --- a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java +++ b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java @@ -14,14 +14,14 @@ */ package software.amazon.awssdk.http.urlconnection; -import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely; import static software.amazon.awssdk.utils.FunctionalUtils.safeFunction; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.io.UncheckedIOException; import java.net.HttpURLConnection; import java.net.ProtocolException; import java.net.URL; @@ -29,10 +29,12 @@ import java.util.List; import java.util.Map; import java.util.function.Function; +import org.apache.logging.log4j.Level; import org.junit.Ignore; import org.junit.Test; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientTestSuite; +import software.amazon.awssdk.testutils.LogCaptor; public final class UrlConnectionHttpClientWithCustomCreateWireMockTest extends SdkHttpClientTestSuite { @@ -100,7 +102,9 @@ public OutputStream getOutputStream() { }); assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) - .isInstanceOf(UncheckedIOException.class); + .isInstanceOf(IOException.class) + .hasMessage("Unexpected NullPointerException when calling HttpURLConnection") + .hasCauseInstanceOf(RuntimeException.class); } @Test @@ -113,7 +117,118 @@ public InputStream getInputStream() { }); assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) - .isInstanceOf(UncheckedIOException.class); + .isInstanceOf(IOException.class) + .hasMessage("Unexpected NullPointerException when calling HttpURLConnection") + .hasCauseInstanceOf(RuntimeException.class); + } + + @Test + public void testGetOutputStreamBareNpeIsWrappedAsIo() throws Exception { + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public OutputStream getOutputStream() { + throw new NullPointerException("this.http is null"); + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) + .isInstanceOf(IOException.class) + .hasMessage("Unexpected NullPointerException when calling HttpURLConnection") + .hasCauseInstanceOf(NullPointerException.class); + } + + @Test + public void testGetInputStreamBareNpeIsWrappedAsIo() throws Exception { + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public InputStream getInputStream() { + throw new NullPointerException("this.http is null"); + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) + .isInstanceOf(IOException.class) + .hasMessage("Unexpected NullPointerException when calling HttpURLConnection") + .hasCauseInstanceOf(NullPointerException.class); + } + + @Test + public void testGetOutputStreamStacklessNpeIsWrappedAsIo() throws Exception { + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public OutputStream getOutputStream() { + throw stacklessNpe(); + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) + .isInstanceOf(IOException.class) + .hasCauseInstanceOf(NullPointerException.class); + } + + @Test + public void testGetOutputStreamUnrelatedNpeIsWrappedAsIo() throws Exception { + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public OutputStream getOutputStream() { + throw new NullPointerException("custom connection failure"); + } + }); + + try (LogCaptor logCaptor = LogCaptor.create(Level.DEBUG)) { + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) + .isInstanceOf(IOException.class) + .hasCauseInstanceOf(NullPointerException.class); + assertThat(logCaptor.loggedEvents()).anySatisfy(logEvent -> + assertThat(logEvent.getMessage().getFormattedMessage()) + .contains("Converting NPE from HttpURLConnection implementation") + .contains("to IOException for retry evaluation")); + } + } + + @Test + public void testGetOutputStreamNonNpeRuntimeExceptionIsNotWrapped() throws Exception { + RuntimeException expected = new IllegalStateException("custom connection failure"); + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public OutputStream getOutputStream() { + throw expected; + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)).isSameAs(expected); + } + + @Test + public void testGetOutputStreamIOExceptionRemainsChecked() throws Exception { + IOException expected = new IOException("connection closed"); + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public OutputStream getOutputStream() throws IOException { + throw expected; + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)).isSameAs(expected); + } + + @Test + public void testGetInputStreamIOExceptionRemainsChecked() throws Exception { + IOException expected = new IOException("connection closed"); + connectionInterceptor = safeFunction(connection -> new DelegateHttpURLConnection(connection) { + @Override + public InputStream getInputStream() throws IOException { + throw expected; + } + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)).isSameAs(expected); + } + + private static NullPointerException stacklessNpe() { + NullPointerException result = new NullPointerException(); + result.setStackTrace(new StackTraceElement[0]); + return result; } private class DelegateHttpURLConnection extends HttpURLConnection {