Include channel diagnostics in Read/Write timeout error messages in n…

.changes/next-release/bugfix-NettyNIOHTTPClient-ce5dc9d.json

@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "Netty NIO HTTP Client",
+    "contributor": "",
+    "description": "Include channel diagnostics in Read/Write timeout error messages to aid debugging."
+}

http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java

@@ -78,14 +78,23 @@ private NettyUtils() {
     public static Throwable decorateException(Channel channel, Throwable originalCause) {
         if (isAcquireTimeoutException(originalCause)) {
             return new Throwable(getMessageForAcquireTimeoutException(), originalCause);
-        } else if (isTooManyPendingAcquiresException(originalCause)) {
+        }
+
+        if (isTooManyPendingAcquiresException(originalCause)) {
             return new Throwable(getMessageForTooManyAcquireOperationsError(), originalCause);
-        } else if (originalCause instanceof ReadTimeoutException) {
-            return new IOException("Read timed out", originalCause);
-        } else if (originalCause instanceof WriteTimeoutException) {
-            return new IOException("Write timed out", originalCause);
-        } else if (originalCause instanceof ClosedChannelException || isConnectionResetException(originalCause)) {
-            return new IOException(NettyUtils.closedChannelMessage(channel), originalCause);
+        }
+
+        if (originalCause instanceof ReadTimeoutException) {
+            return new IOException(errorMessageWithChannelDiagnostics(channel, "Read timed out"),
+                                   originalCause);
+        }
+
+        if (originalCause instanceof WriteTimeoutException) {
+            return new IOException(errorMessageWithChannelDiagnostics(channel, "Write timed out"), originalCause);
+        }
+
+        if (originalCause instanceof ClosedChannelException || isConnectionResetException(originalCause)) {
+            return new IOException(closedChannelMessage(channel), originalCause);
         }
 
         return originalCause;
@@ -152,15 +161,15 @@ private static String getMessageForTooManyAcquireOperationsError() {
                + "AWS, or by increasing the number of hosts sending requests.";
     }
 
-    public static String closedChannelMessage(Channel channel) {
+    public static String errorMessageWithChannelDiagnostics(Channel channel, String message) {
         ChannelDiagnostics channelDiagnostics = channel != null && channel.attr(CHANNEL_DIAGNOSTICS) != null ?
                                                 channel.attr(CHANNEL_DIAGNOSTICS).get() : null;
         ChannelDiagnostics parentChannelDiagnostics = channel != null && channel.parent() != null && 
                                                       channel.parent().attr(CHANNEL_DIAGNOSTICS) != null ?
                                                       channel.parent().attr(CHANNEL_DIAGNOSTICS).get() : null;
 
         StringBuilder error = new StringBuilder();
-        error.append(CLOSED_CHANNEL_ERROR_MESSAGE);
+        error.append(message);
 
         if (channelDiagnostics != null) {
             error.append(" Channel Information: ").append(channelDiagnostics);
@@ -173,6 +182,10 @@ public static String closedChannelMessage(Channel channel) {
         return error.toString();
     }
 
+    public static String closedChannelMessage(Channel channel) {
+        return errorMessageWithChannelDiagnostics(channel, CLOSED_CHANNEL_ERROR_MESSAGE);
+    }
+
     /**
      * Creates a {@link BiConsumer} that notifies the promise of any failures either via the {@link Throwable} passed into the
      * BiConsumer of as a result of running the successFunction.

http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtilsTest.java

@@ -43,12 +43,17 @@
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
 import javax.net.ssl.SSLEngine;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mockito;
 import org.slf4j.Logger;
+import software.amazon.awssdk.http.nio.netty.internal.ChannelDiagnostics;
 import software.amazon.awssdk.http.nio.netty.internal.MockChannel;
 
 public class NettyUtilsTest {
@@ -274,108 +279,96 @@
             .isEqualTo(NettyUtils.CLOSED_CHANNEL_ERROR_MESSAGE);
     }
 
-    @Test
-    public void decorateException_with_TimeoutException() {
+    private static Stream<Arguments> decorateExceptionWrappedCases() {
+        return Stream.of(
+            Arguments.of(new TimeoutException("...Acquire operation took longer..."),
+                         Throwable.class, TimeoutException.class),
+            Arguments.of(new IllegalStateException("...Too many outstanding acquire operations..."),
+                         Throwable.class, IllegalStateException.class),
+            Arguments.of(new ReadTimeoutException(),
+                         IOException.class, ReadTimeoutException.class),
+            Arguments.of(new WriteTimeoutException(),
+                         IOException.class, WriteTimeoutException.class),
+            Arguments.of(new ClosedChannelException(),
+                         IOException.class, ClosedChannelException.class),
+            Arguments.of(new IOException("...Connection reset by peer..."),
+                         IOException.class, IOException.class)
+        );
+    }
 
+    @ParameterizedTest
+    @MethodSource("decorateExceptionWrappedCases")
+    public void decorateException_wrapsException(Throwable input,
+                                                 Class<? extends Throwable> expectedType,
+                                                 Class<? extends Throwable> expectedCauseType) {
         Channel channel = mock(Channel.class);
-        Throwable timeoutException = new TimeoutException("...Acquire operation took longer...");
-        Throwable output = NettyUtils.decorateException(channel, timeoutException);
+        Throwable output = NettyUtils.decorateException(channel, input);
 
-        assertThat(output).isInstanceOf(Throwable.class);
-        assertThat(output.getCause()).isInstanceOf(TimeoutException.class);
+        assertThat(output).isInstanceOf(expectedType);
+        assertThat(output.getCause()).isInstanceOf(expectedCauseType);
         assertThat(output.getMessage()).isNotNull();
     }
 
-    @Test
-    public void decorateException_with_TimeoutException_noMsg() {
-
-        Channel channel = mock(Channel.class);
-        Throwable timeoutException = new TimeoutException();
-        Throwable output = NettyUtils.decorateException(channel, timeoutException);
-
-        assertThat(output).isInstanceOf(TimeoutException.class);
-        assertThat(output.getCause()).isNull();
+    private static Stream<Arguments> decorateExceptionPassthroughCases() {
+        return Stream.of(
+            Arguments.of(new TimeoutException()),
+            Arguments.of(new IllegalStateException()),
+            Arguments.of(new IOException())
+        );
     }
 
-    @Test
-    public void decorateException_with_IllegalStateException() {
-
+    @ParameterizedTest
+    @MethodSource("decorateExceptionPassthroughCases")
+    public void decorateException_noMatchingMessage_returnsOriginal(Throwable input) {
         Channel channel = mock(Channel.class);
-        Throwable illegalStateException = new IllegalStateException("...Too many outstanding acquire operations...");
-        Throwable output = NettyUtils.decorateException(channel, illegalStateException);
+        Throwable output = NettyUtils.decorateException(channel, input);
 
-        assertThat(output).isInstanceOf(Throwable.class);
-        assertThat(output.getCause()).isInstanceOf(IllegalStateException.class);
-        assertThat(output.getMessage()).isNotNull();
+        assertThat(output).isSameAs(input);
     }
 
-    @Test
-    public void decorateException_with_IllegalStateException_noMsg() {
-
-        Channel channel = mock(Channel.class);
-        Throwable illegalStateException = new IllegalStateException();
-        Throwable output = NettyUtils.decorateException(channel, illegalStateException);
-
-        assertThat(output).isInstanceOf(IllegalStateException.class);
-        assertThat(output.getCause()).isNull();
+    private static Stream<Arguments> channelDiagnosticsExceptionCases() {
+        return Stream.of(
+            Arguments.of(new ReadTimeoutException(), "Read timed out"),
+            Arguments.of(new WriteTimeoutException(), "Write timed out")
+        );
     }
 
-    @Test
-    public void decorateException_with_ReadTimeoutException() {
-
+    @ParameterizedTest
+    @MethodSource("channelDiagnosticsExceptionCases")
+    public void decorateException_includesChannelDiagnostics(Throwable input, String expectedPrefix) {
         Channel channel = mock(Channel.class);
-        Throwable readTimeoutException = new ReadTimeoutException();
-        Throwable output = NettyUtils.decorateException(channel, readTimeoutException);
-
-        assertThat(output).isInstanceOf(IOException.class);
-        assertThat(output.getCause()).isInstanceOf(ReadTimeoutException.class);
-        assertThat(output.getMessage()).isNotNull();
-    }
-
-    @Test
-    public void decorateException_with_WriteTimeoutException() {
+        Attribute attribute = mock(Attribute.class);
+        ChannelDiagnostics diagnostics = new ChannelDiagnostics(channel);
+        when(channel.attr(any())).thenReturn(attribute);
+        when(attribute.get()).thenReturn(diagnostics);
+        when(channel.parent()).thenReturn(null);
 
-        Channel channel = mock(Channel.class);
-        Throwable writeTimeoutException = new WriteTimeoutException();
-        Throwable output = NettyUtils.decorateException(channel, writeTimeoutException);
+        Throwable output = NettyUtils.decorateException(channel, input);
 
         assertThat(output).isInstanceOf(IOException.class);
-        assertThat(output.getCause()).isInstanceOf(WriteTimeoutException.class);
-        assertThat(output.getMessage()).isNotNull();
+        assertThat(output.getMessage()).startsWith(expectedPrefix);
+        assertThat(output.getMessage()).contains("Channel Information:");
     }
 
     @Test
-    public void decorateException_with_ClosedChannelException() {
-
-        Channel channel = mock(Channel.class);
-        Throwable closedChannelException = new ClosedChannelException();
-        Throwable output = NettyUtils.decorateException(channel, closedChannelException);
-
-        assertThat(output).isInstanceOf(IOException.class);
-        assertThat(output.getCause()).isInstanceOf(ClosedChannelException.class);
-        assertThat(output.getMessage()).isNotNull();
+    public void errorMessageWithChannelDiagnostics_nullChannel_returnsBaseMessage() {
+        assertThat(NettyUtils.errorMessageWithChannelDiagnostics(null, "test message"))
+            .isEqualTo("test message");
     }
 
     @Test
-    public void decorateException_with_IOException_reset() {
-
+    public void errorMessageWithChannelDiagnostics_withDiagnostics_appendsChannelInfo() {
         Channel channel = mock(Channel.class);
-        Throwable closedChannelException = new IOException("...Connection reset by peer...");
-        Throwable output = NettyUtils.decorateException(channel, closedChannelException);
-
-        assertThat(output).isInstanceOf(IOException.class);
-        assertThat(output.getCause()).isInstanceOf(IOException.class);
-        assertThat(output.getMessage()).isNotNull();
-    }
-
-    @Test
-    public void decorateException_with_IOException_noMsg() {
+        Attribute attribute = mock(Attribute.class);
+        ChannelDiagnostics diagnostics = new ChannelDiagnostics(channel);
+        when(channel.attr(any())).thenReturn(attribute);
+        when(attribute.get()).thenReturn(diagnostics);
+        when(channel.parent()).thenReturn(null);
 
-        Channel channel = mock(Channel.class);
-        Throwable closedChannelException = new IOException();
-        Throwable output = NettyUtils.decorateException(channel, closedChannelException);
+        String result = NettyUtils.errorMessageWithChannelDiagnostics(channel, "custom error");
 
-        assertThat(output).isInstanceOf(IOException.class);
-        assertThat(output.getCause()).isNull();
+        assertThat(result).startsWith("custom error");
+        assertThat(result).contains("Channel Information:");
     }
+
 }