Skip to content

Commit 271041c

Browse files
zoewanggzoewangg
authored
Include channel diagnostics in Read/Write timeout error messages in netty (#6839)
1 parent 720dbf0 commit 271041c

File tree

3 files changed

+97
-85
lines changed

3 files changed

+97
-85
lines changed

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "Netty NIO HTTP Client",
4+
"contributor": "",
5+
"description": "Include channel diagnostics in Read/Write timeout error messages to aid debugging."
6+
}

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

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,23 @@ private NettyUtils() {
7878
public static Throwable decorateException(Channel channel, Throwable originalCause) {
7979
if (isAcquireTimeoutException(originalCause)) {
8080
return new Throwable(getMessageForAcquireTimeoutException(), originalCause);
81-
} else if (isTooManyPendingAcquiresException(originalCause)) {
81+
}
82+
83+
if (isTooManyPendingAcquiresException(originalCause)) {
8284
return new Throwable(getMessageForTooManyAcquireOperationsError(), originalCause);
83-
} else if (originalCause instanceof ReadTimeoutException) {
84-
return new IOException("Read timed out", originalCause);
85-
} else if (originalCause instanceof WriteTimeoutException) {
86-
return new IOException("Write timed out", originalCause);
87-
} else if (originalCause instanceof ClosedChannelException || isConnectionResetException(originalCause)) {
88-
return new IOException(NettyUtils.closedChannelMessage(channel), originalCause);
85+
}
86+
87+
if (originalCause instanceof ReadTimeoutException) {
88+
return new IOException(errorMessageWithChannelDiagnostics(channel, "Read timed out"),
89+
originalCause);
90+
}
91+
92+
if (originalCause instanceof WriteTimeoutException) {
93+
return new IOException(errorMessageWithChannelDiagnostics(channel, "Write timed out"), originalCause);
94+
}
95+
96+
if (originalCause instanceof ClosedChannelException || isConnectionResetException(originalCause)) {
97+
return new IOException(closedChannelMessage(channel), originalCause);
8998
}
9099

91100
return originalCause;
@@ -152,15 +161,15 @@ private static String getMessageForTooManyAcquireOperationsError() {
152161
+ "AWS, or by increasing the number of hosts sending requests.";
153162
}
154163

155-
public static String closedChannelMessage(Channel channel) {
164+
public static String errorMessageWithChannelDiagnostics(Channel channel, String message) {
156165
ChannelDiagnostics channelDiagnostics = channel != null && channel.attr(CHANNEL_DIAGNOSTICS) != null ?
157166
channel.attr(CHANNEL_DIAGNOSTICS).get() : null;
158167
ChannelDiagnostics parentChannelDiagnostics = channel != null && channel.parent() != null &&
159168
channel.parent().attr(CHANNEL_DIAGNOSTICS) != null ?
160169
channel.parent().attr(CHANNEL_DIAGNOSTICS).get() : null;
161170

162171
StringBuilder error = new StringBuilder();
163-
error.append(CLOSED_CHANNEL_ERROR_MESSAGE);
172+
error.append(message);
164173

165174
if (channelDiagnostics != null) {
166175
error.append(" Channel Information: ").append(channelDiagnostics);
@@ -173,6 +182,10 @@ public static String closedChannelMessage(Channel channel) {
173182
return error.toString();
174183
}
175184

185+
public static String closedChannelMessage(Channel channel) {
186+
return errorMessageWithChannelDiagnostics(channel, CLOSED_CHANNEL_ERROR_MESSAGE);
187+
}
188+
176189
/**
177190
* Creates a {@link BiConsumer} that notifies the promise of any failures either via the {@link Throwable} passed into the
178191
* 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‎

Lines changed: 69 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,17 @@
4343
import java.util.concurrent.TimeoutException;
4444
import java.util.concurrent.atomic.AtomicBoolean;
4545
import java.util.concurrent.atomic.AtomicReference;
46+
import java.util.stream.Stream;
4647
import javax.net.ssl.SSLEngine;
4748
import org.junit.jupiter.api.AfterAll;
4849
import org.junit.jupiter.api.BeforeAll;
4950
import org.junit.jupiter.api.Test;
51+
import org.junit.jupiter.params.ParameterizedTest;
52+
import org.junit.jupiter.params.provider.Arguments;
53+
import org.junit.jupiter.params.provider.MethodSource;
5054
import org.mockito.Mockito;
5155
import org.slf4j.Logger;
56+
import software.amazon.awssdk.http.nio.netty.internal.ChannelDiagnostics;
5257
import software.amazon.awssdk.http.nio.netty.internal.MockChannel;
5358

5459
public class NettyUtilsTest {
@@ -274,108 +279,96 @@ public void closedChannelMessage_with_nullParentChannelAttribute() throws Except
274279
.isEqualTo(NettyUtils.CLOSED_CHANNEL_ERROR_MESSAGE);
275280
}
276281

277-
@Test
278-
public void decorateException_with_TimeoutException() {
282+
private static Stream<Arguments> decorateExceptionWrappedCases() {
283+
return Stream.of(
284+
Arguments.of(new TimeoutException("...Acquire operation took longer..."),
285+
Throwable.class, TimeoutException.class),
286+
Arguments.of(new IllegalStateException("...Too many outstanding acquire operations..."),
287+
Throwable.class, IllegalStateException.class),
288+
Arguments.of(new ReadTimeoutException(),
289+
IOException.class, ReadTimeoutException.class),
290+
Arguments.of(new WriteTimeoutException(),
291+
IOException.class, WriteTimeoutException.class),
292+
Arguments.of(new ClosedChannelException(),
293+
IOException.class, ClosedChannelException.class),
294+
Arguments.of(new IOException("...Connection reset by peer..."),
295+
IOException.class, IOException.class)
296+
);
297+
}
279298

299+
@ParameterizedTest
300+
@MethodSource("decorateExceptionWrappedCases")
301+
public void decorateException_wrapsException(Throwable input,
302+
Class<? extends Throwable> expectedType,
303+
Class<? extends Throwable> expectedCauseType) {
280304
Channel channel = mock(Channel.class);
281-
Throwable timeoutException = new TimeoutException("...Acquire operation took longer...");
282-
Throwable output = NettyUtils.decorateException(channel, timeoutException);
305+
Throwable output = NettyUtils.decorateException(channel, input);
283306

284-
assertThat(output).isInstanceOf(Throwable.class);
285-
assertThat(output.getCause()).isInstanceOf(TimeoutException.class);
307+
assertThat(output).isInstanceOf(expectedType);
308+
assertThat(output.getCause()).isInstanceOf(expectedCauseType);
286309
assertThat(output.getMessage()).isNotNull();
287310
}
288311

289-
@Test
290-
public void decorateException_with_TimeoutException_noMsg() {
291-
292-
Channel channel = mock(Channel.class);
293-
Throwable timeoutException = new TimeoutException();
294-
Throwable output = NettyUtils.decorateException(channel, timeoutException);
295-
296-
assertThat(output).isInstanceOf(TimeoutException.class);
297-
assertThat(output.getCause()).isNull();
312+
private static Stream<Arguments> decorateExceptionPassthroughCases() {
313+
return Stream.of(
314+
Arguments.of(new TimeoutException()),
315+
Arguments.of(new IllegalStateException()),
316+
Arguments.of(new IOException())
317+
);
298318
}
299319

300-
@Test
301-
public void decorateException_with_IllegalStateException() {
302-
320+
@ParameterizedTest
321+
@MethodSource("decorateExceptionPassthroughCases")
322+
public void decorateException_noMatchingMessage_returnsOriginal(Throwable input) {
303323
Channel channel = mock(Channel.class);
304-
Throwable illegalStateException = new IllegalStateException("...Too many outstanding acquire operations...");
305-
Throwable output = NettyUtils.decorateException(channel, illegalStateException);
324+
Throwable output = NettyUtils.decorateException(channel, input);
306325

307-
assertThat(output).isInstanceOf(Throwable.class);
308-
assertThat(output.getCause()).isInstanceOf(IllegalStateException.class);
309-
assertThat(output.getMessage()).isNotNull();
326+
assertThat(output).isSameAs(input);
310327
}
311328

312-
@Test
313-
public void decorateException_with_IllegalStateException_noMsg() {
314-
315-
Channel channel = mock(Channel.class);
316-
Throwable illegalStateException = new IllegalStateException();
317-
Throwable output = NettyUtils.decorateException(channel, illegalStateException);
318-
319-
assertThat(output).isInstanceOf(IllegalStateException.class);
320-
assertThat(output.getCause()).isNull();
329+
private static Stream<Arguments> channelDiagnosticsExceptionCases() {
330+
return Stream.of(
331+
Arguments.of(new ReadTimeoutException(), "Read timed out"),
332+
Arguments.of(new WriteTimeoutException(), "Write timed out")
333+
);
321334
}
322335

323-
@Test
324-
public void decorateException_with_ReadTimeoutException() {
325-
336+
@ParameterizedTest
337+
@MethodSource("channelDiagnosticsExceptionCases")
338+
public void decorateException_includesChannelDiagnostics(Throwable input, String expectedPrefix) {
326339
Channel channel = mock(Channel.class);
327-
Throwable readTimeoutException = new ReadTimeoutException();
328-
Throwable output = NettyUtils.decorateException(channel, readTimeoutException);
329-
330-
assertThat(output).isInstanceOf(IOException.class);
331-
assertThat(output.getCause()).isInstanceOf(ReadTimeoutException.class);
332-
assertThat(output.getMessage()).isNotNull();
333-
}
334-
335-
@Test
336-
public void decorateException_with_WriteTimeoutException() {
340+
Attribute attribute = mock(Attribute.class);
341+
ChannelDiagnostics diagnostics = new ChannelDiagnostics(channel);
342+
when(channel.attr(any())).thenReturn(attribute);
343+
when(attribute.get()).thenReturn(diagnostics);
344+
when(channel.parent()).thenReturn(null);
337345

338-
Channel channel = mock(Channel.class);
339-
Throwable writeTimeoutException = new WriteTimeoutException();
340-
Throwable output = NettyUtils.decorateException(channel, writeTimeoutException);
346+
Throwable output = NettyUtils.decorateException(channel, input);
341347

342348
assertThat(output).isInstanceOf(IOException.class);
343-
assertThat(output.getCause()).isInstanceOf(WriteTimeoutException.class);
344-
assertThat(output.getMessage()).isNotNull();
349+
assertThat(output.getMessage()).startsWith(expectedPrefix);
350+
assertThat(output.getMessage()).contains("Channel Information:");
345351
}
346352

347353
@Test
348-
public void decorateException_with_ClosedChannelException() {
349-
350-
Channel channel = mock(Channel.class);
351-
Throwable closedChannelException = new ClosedChannelException();
352-
Throwable output = NettyUtils.decorateException(channel, closedChannelException);
353-
354-
assertThat(output).isInstanceOf(IOException.class);
355-
assertThat(output.getCause()).isInstanceOf(ClosedChannelException.class);
356-
assertThat(output.getMessage()).isNotNull();
354+
public void errorMessageWithChannelDiagnostics_nullChannel_returnsBaseMessage() {
355+
assertThat(NettyUtils.errorMessageWithChannelDiagnostics(null, "test message"))
356+
.isEqualTo("test message");
357357
}
358358

359359
@Test
360-
public void decorateException_with_IOException_reset() {
361-
360+
public void errorMessageWithChannelDiagnostics_withDiagnostics_appendsChannelInfo() {
362361
Channel channel = mock(Channel.class);
363-
Throwable closedChannelException = new IOException("...Connection reset by peer...");
364-
Throwable output = NettyUtils.decorateException(channel, closedChannelException);
365-
366-
assertThat(output).isInstanceOf(IOException.class);
367-
assertThat(output.getCause()).isInstanceOf(IOException.class);
368-
assertThat(output.getMessage()).isNotNull();
369-
}
370-
371-
@Test
372-
public void decorateException_with_IOException_noMsg() {
362+
Attribute attribute = mock(Attribute.class);
363+
ChannelDiagnostics diagnostics = new ChannelDiagnostics(channel);
364+
when(channel.attr(any())).thenReturn(attribute);
365+
when(attribute.get()).thenReturn(diagnostics);
366+
when(channel.parent()).thenReturn(null);
373367

374-
Channel channel = mock(Channel.class);
375-
Throwable closedChannelException = new IOException();
376-
Throwable output = NettyUtils.decorateException(channel, closedChannelException);
368+
String result = NettyUtils.errorMessageWithChannelDiagnostics(channel, "custom error");
377369

378-
assertThat(output).isInstanceOf(IOException.class);
379-
assertThat(output.getCause()).isNull();
370+
assertThat(result).startsWith("custom error");
371+
assertThat(result).contains("Channel Information:");
380372
}
373+
381374
}

0 commit comments

Comments
 (0)