Add null check for channel when generating error message #3574

sigurjonviktorsson · 2022-11-29T13:41:59Z

Motivation and Context

I think this issue is relevant #3435

There's a possibility of channel being null if the channel acquisition fails in this code path:
https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java#L175

2022-11-29 13:35:37,629  WARN  [aws-java-sdk-NettyEventLoop-1-0] io.netty.channel.AbstractChannelHandlerContext - An exception 'java.lang.NullPointerException' [enable DEBUG level for full stacktrace] was thrown by a user handler's exceptionCaught() method while handling the following exception:
software.amazon.awssdk.http.nio.netty.internal.FutureCancelledException: software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException: HTTP request execution did not complete before the specified timeout configuration: 80 millis
        at software.amazon.awssdk.http.nio.netty.internal.NettyRequestExecutor.lambda$null$3(NettyRequestExecutor.java:136)
        at io.netty.util.concurrent.PromiseTask.runTask(PromiseTask.java:98)
        at io.netty.util.concurrent.PromiseTask.run(PromiseTask.java:106)
        at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174)
        at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167)
        at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
        at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:500)
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:995)
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException: HTTP request execution did not complete before the specified timeout configuration: 80 millis
        at software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException$BuilderImpl.build(ApiCallAttemptTimeoutException.java:88)
        at software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException.create(ApiCallAttemptTimeoutException.java:38)
        at software.amazon.awssdk.core.internal.http.pipeline.stages.MakeAsyncHttpRequestStage.lambda$setupAttemptTimer$5(MakeAsyncHttpRequestStage.java:214)
        at software.amazon.awssdk.core.internal.http.timers.AsyncTimeoutTask.run(AsyncTimeoutTask.java:52)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        ... 1 common frames omitted

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

sigurjonviktorsson · 2022-12-01T00:08:16Z

@zoewangg How can we move this forward?

zoewangg

LGTM. Thank you for your contribution!

zoewangg · 2022-12-01T00:31:43Z

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

-        ChannelDiagnostics channelDiagnostics = channel.attr(CHANNEL_DIAGNOSTICS).get();
-        ChannelDiagnostics parentChannelDiagnostics = channel.parent() != null ? channel.parent().attr(CHANNEL_DIAGNOSTICS).get()
-                                                                               : null;
+        ChannelDiagnostics channelDiagnostics, parentChannelDiagnostics;


Looks like compilation failed.

ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project netty-nio-client: Compilation failure: Compilation failure: NettyUtils.java:[148,12] error: variable channelDiagnostics might not have been initialized NettyUtils.java:[151,16] error: variable parentChannelDiagnostics might not have been initialized

Should be fixed now.

I think checkstyle still failed. Are you able to build it locally?

How do I run checkstyle?

mvn install is failing locally on something unrelated I think:

There were some compilation issues with NettyUtilsTest , I have fixed and updated the PR.
The builds are passing now

Dismissing review because build failed

sonarqubecloud · 2022-12-01T23:47:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

75.0% Coverage
0.0% Duplication

sigurjonviktorsson · 2022-12-19T13:33:59Z

@zoewangg Can you take another look?

zoewangg · 2022-12-20T22:12:50Z

Apologies for the lack of response. I think checkstyle still failed (Each variable declaration must be in its own statement. [MultipleVariableDeclarations]). The variable should be declared in separate statements as below.

        ChannelDiagnostics channelDiagnostics = null;
        ChannelDiagnostics parentChannelDiagnostics = null;

What JDK version are you using to build the SDK? I think there are some false positive checkstyle issues when building with JDK 18+ and we will work on fixing it.

joviegas · 2022-12-23T21:08:44Z

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

@@ -133,9 +133,11 @@ private static String getMessageForTooManyAcquireOperationsError() {
    }

    public static String closedChannelMessage(Channel channel) {
-        ChannelDiagnostics channelDiagnostics = channel.attr(CHANNEL_DIAGNOSTICS).get();


what about updating it to below code

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).get() : null;

That looks good to me, I added a null check for the CHANNEL_DIAGNOSTICS attribute on the parent as well. Not sure if this passes style rules.

joviegas · 2022-12-23T21:10:01Z

Can we add Junit test cases in NettyUtilTest.java

    @Test
    public void closedChannelMessage_with_nullChannelAttribute() throws Exception {

        Channel channel = Mockito.mock(Channel.class);
        when(channel.parent()).thenReturn(null);

        assertThat(NettyUtils.closedChannelMessage(channel))
            .isEqualTo(NettyUtils.CLOSED_CHANNEL_ERROR_MESSAGE);
    }

    @Test
    public void closedChannelMessage_with_nullChannel() throws Exception {
        Channel channel = null;
        assertThat(NettyUtils.closedChannelMessage(channel))
            .isEqualTo(NettyUtils.CLOSED_CHANNEL_ERROR_MESSAGE);
    }


    @Test
    public void closedChannelMessage_with_nullParentChannel() throws Exception {

        Channel channel = Mockito.mock(Channel.class);
        Attribute attribute = Mockito.mock(Attribute.class);
        when(channel.parent()).thenReturn(null);
        when(channel.attr(Mockito.any())).thenReturn(attribute);

        assertThat(NettyUtils.closedChannelMessage(channel))
            .isEqualTo(NettyUtils.CLOSED_CHANNEL_ERROR_MESSAGE);
    }

…n it

sonarqubecloud · 2023-01-09T23:52:45Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

93.8% Coverage
0.0% Duplication

sigurjonviktorsson requested a review from a team as a code owner November 29, 2022 13:42

zoewangg previously approved these changes Dec 1, 2022

View reviewed changes

zoewangg enabled auto-merge (squash) December 1, 2022 00:26

zoewangg disabled auto-merge December 1, 2022 00:30

zoewangg reviewed Dec 1, 2022

View reviewed changes

sigurjonviktorsson requested a review from zoewangg December 1, 2022 22:50

joviegas reviewed Dec 23, 2022

View reviewed changes

sigurjonviktorsson and others added 8 commits January 5, 2023 23:03

Add check for channel being null before trying to access properties o…

bbeba47

…n it

update changelog

da9955e

Update NettyUtils.java

15a0deb

Update NettyUtils.java

43dd735

Update NettyUtils.java

94809f2

Update NettyUtils.java

2c8db95

Update NettyUtilsTest.java

13e9a81

Update NettyUtilsTest.java

beff89c

joviegas force-pushed the sigurjonviktorsson-handle-null-channel branch from 242a74c to 3e21ec5 Compare January 6, 2023 05:20

Added CLOSED_CHANNEL_ERROR_MESSAGE to fix compilation issues

2063abf

joviegas force-pushed the sigurjonviktorsson-handle-null-channel branch from 3e21ec5 to 2063abf Compare January 6, 2023 05:24

Merge branch 'master' into sigurjonviktorsson-handle-null-channel

3e1c3cb

zoewangg enabled auto-merge (squash) January 9, 2023 22:01

zoewangg approved these changes Jan 9, 2023

View reviewed changes

joviegas disabled auto-merge January 10, 2023 01:16

joviegas enabled auto-merge (squash) January 10, 2023 01:17

joviegas merged commit 492dfdd into aws:master Jan 10, 2023

zoewangg mentioned this pull request Jan 13, 2023

NullpointerException in NettyUtils.closedChannelMessage #3435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add null check for channel when generating error message #3574

Add null check for channel when generating error message #3574

Uh oh!

sigurjonviktorsson commented Nov 29, 2022 •

edited

Loading

Uh oh!

sigurjonviktorsson commented Dec 1, 2022

Uh oh!

zoewangg left a comment

Uh oh!

zoewangg Dec 1, 2022

Uh oh!

sigurjonviktorsson Dec 1, 2022

Uh oh!

zoewangg Dec 2, 2022

Uh oh!

sigurjonviktorsson Dec 5, 2022

Uh oh!

joviegas Jan 6, 2023

Uh oh!

sonarqubecloud bot commented Dec 1, 2022

Uh oh!

sigurjonviktorsson commented Dec 19, 2022

Uh oh!

zoewangg commented Dec 20, 2022

Uh oh!

joviegas Dec 23, 2022 •

edited

Loading

Uh oh!

sigurjonviktorsson Jan 3, 2023

Uh oh!

joviegas commented Dec 23, 2022

Uh oh!

sonarqubecloud bot commented Jan 9, 2023

Uh oh!

Uh oh!

Add null check for channel when generating error message #3574

Add null check for channel when generating error message #3574

Uh oh!

Conversation

sigurjonviktorsson commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

sigurjonviktorsson commented Dec 1, 2022

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

zoewangg Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

sigurjonviktorsson Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

zoewangg Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

sigurjonviktorsson Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

joviegas Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 1, 2022

Uh oh!

sigurjonviktorsson commented Dec 19, 2022

Uh oh!

zoewangg commented Dec 20, 2022

Uh oh!

joviegas Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurjonviktorsson Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

joviegas commented Dec 23, 2022

Uh oh!

sonarqubecloud bot commented Jan 9, 2023

Uh oh!

Uh oh!

sigurjonviktorsson commented Nov 29, 2022 •

edited

Loading

joviegas Dec 23, 2022 •

edited

Loading