From 5e86dc946899093d82f69645d4bbcbef46fb8727 Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 30 Jun 2021 14:53:10 -0700 Subject: [PATCH 1/3] [UrlConnectionHttpClient] Wrap erroneous NPE from HttpURLConnection as IOException ## Motivation `HttpURLConnection#getInputStream0` has been observed to intermittently throw `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 should bias towards propagating these as `IOException`s. ## Modifications * When interacting specifically with HttpURLConnection#getResponseCode, wrap any unexpected NullPointerExceptions as IOException * Add corresponding unit test ## Result * Transient errors will be retried and be less likely to surface to a user's application layer --- .../UrlConnectionHttpClient.java | 17 ++++++++++- ...ttpClientWithCustomCreateWireMockTest.java | 29 ++++++++++++++++++- .../awssdk/http/SdkHttpClientTestSuite.java | 2 +- 3 files changed, 45 insertions(+), 3 deletions(-) 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 37840ce1c4ce..b07301c688ea 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 @@ -207,7 +207,7 @@ public HttpExecuteResponse call() throws IOException { request.contentStreamProvider().ifPresent(provider -> invokeSafely(() -> IoUtils.copy(provider.newStream(), connection.getOutputStream()))); - int responseCode = connection.getResponseCode(); + int responseCode = getResponseCodeSafely(connection); boolean isErrorResponse = HttpStatusFamily.of(responseCode).isOneOf(CLIENT_ERROR, SERVER_ERROR); InputStream content = !isErrorResponse ? connection.getInputStream() : connection.getErrorStream(); AbortableInputStream responseBody = content != null ? @@ -224,6 +224,21 @@ public HttpExecuteResponse call() throws IOException { .build(); } + /** + * {@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. + */ + private static int getResponseCodeSafely(HttpURLConnection connection) throws IOException { + Validate.paramNotNull(connection, "connection"); + try { + return connection.getResponseCode(); + } catch (NullPointerException e) { + throw new IOException("Unexpected NullPointerException when trying to read response from HttpURLConnection", e); + } + } + private Map> extractHeaders(HttpURLConnection response) { return response.getHeaderFields().entrySet().stream() .filter(e -> e.getKey() != null) 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 6ce76ee84c70..e64392dee52f 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,17 +14,30 @@ */ package software.amazon.awssdk.http.urlconnection; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.spy; import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely; +import static software.amazon.awssdk.utils.FunctionalUtils.safeFunction; +import java.io.IOException; import java.net.HttpURLConnection; +import java.util.function.Function; import org.junit.Ignore; +import org.junit.Test; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientTestSuite; public final class UrlConnectionHttpClientWithCustomCreateWireMockTest extends SdkHttpClientTestSuite { + + private Function connectionInterceptor = Function.identity(); + @Override protected SdkHttpClient createSdkHttpClient(SdkHttpClientOptions options) { - return UrlConnectionHttpClient.create((uri) -> invokeSafely(() -> (HttpURLConnection) uri.toURL().openConnection())); + return UrlConnectionHttpClient.create(uri -> invokeSafely(() -> { + HttpURLConnection connection = (HttpURLConnection) uri.toURL().openConnection(); + return connectionInterceptor.apply(connection); + })); } @Ignore // Not supported when using custom factory @@ -41,4 +54,18 @@ public void testTrustAllWorks() { @Override public void testCustomTlsTrustManagerAndTrustAllFails() { } + + @Test + public void testGetResponseCodeNpeIsWrappedAsIo() throws Exception { + connectionInterceptor = safeFunction(connection -> { + connection = spy(connection); + doThrow(new NullPointerException()).when(connection).getResponseCode(); + return connection; + }); + + assertThatThrownBy(() -> testForResponseCode(HttpURLConnection.HTTP_OK)) + .isInstanceOf(IOException.class) + .hasMessage("Unexpected NullPointerException when trying to read response from HttpURLConnection") + .hasCauseInstanceOf(NullPointerException.class); + } } diff --git a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java index a9f1feacec23..074fd75bec12 100644 --- a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java +++ b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java @@ -152,7 +152,7 @@ public void testCustomTlsTrustManagerAndTrustAllFails() throws Exception { assertThatThrownBy(() -> createSdkHttpClient(httpClientOptions)).isInstanceOf(IllegalArgumentException.class); } - private void testForResponseCode(int returnCode) throws Exception { + protected void testForResponseCode(int returnCode) throws Exception { testForResponseCode(returnCode, SdkHttpMethod.POST); } From cd84ff1e6addc479f5d7d8fea4544a3ae5a83a2b Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 30 Jun 2021 15:07:43 -0700 Subject: [PATCH 2/3] [UrlConnectionHttpClient] Wrap erroneous NPE from HttpURLConnection as IOException ## Motivation `HttpURLConnection#getInputStream0` has been observed to intermittently throw `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 should bias towards propagating these as `IOException`s. ## Modifications * When interacting specifically with HttpURLConnection#getResponseCode, wrap any unexpected NullPointerExceptions as IOException * Add corresponding unit test ## Result * Transient errors will be retried and be less likely to surface to a user's application layer --- .changes/next-release/bugfix-AWSSDKforJavav2-a3c92c2.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-a3c92c2.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-a3c92c2.json b/.changes/next-release/bugfix-AWSSDKforJavav2-a3c92c2.json new file mode 100644 index 000000000000..554e42602d64 --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-a3c92c2.json @@ -0,0 +1,6 @@ +{ + "category": "AWS SDK for Java v2", + "contributor": "Bennett-Lynch", + "type": "bugfix", + "description": "[UrlConnectionHttpClient] Wrap erroneous NPE from HttpURLConnection as IOException" +} From e8898f3940c1db7109c5a05166565156834da2ed Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Wed, 30 Jun 2021 15:28:55 -0700 Subject: [PATCH 3/3] [UrlConnectionHttpClient] Wrap erroneous NPE from HttpURLConnection as IOException ## Motivation `HttpURLConnection#getInputStream0` has been observed to intermittently throw `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 should bias towards propagating these as `IOException`s. ## Modifications * When interacting specifically with HttpURLConnection#getResponseCode, wrap any unexpected NullPointerExceptions as IOException * Add corresponding unit test ## Result * Transient errors will be retried and be less likely to surface to a user's application layer --- .../awssdk/http/urlconnection/UrlConnectionHttpClient.java | 3 +++ 1 file changed, 3 insertions(+) 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 b07301c688ea..b14f00e4e69b 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 @@ -229,6 +229,9 @@ public HttpExecuteResponse call() throws IOException { * {@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. */ private static int getResponseCodeSafely(HttpURLConnection connection) throws IOException { Validate.paramNotNull(connection, "connection");