Bug fix for S3AsyncClient.putObject hangs if there is a connection reset set while uploading of object #3535

joviegas · 2022-11-03T05:20:45Z

Motivation and Context

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset while uploading of objects

Root Cause

PR# 1481 added logic to skip RunError steps in ResponseHandler.#notifyIfResponseNotCompleted.
In the case of PutObject call, the putObject request sends Http100Continue
When Http100Continue is sent HttpResponseStatus CONTINUE = newStatus(100, "Continue") is received , which actually ignores the body, sends the lastByte and immediately starts reading the request stream.
If there was a connection reset because of PR# 1481 it only checked that lastByte was set and thus skipped the Run Error steps.
Since it skipped RunError the error was not propagated and it got stuck.

Modifications

Removed LastHttpContentHandler and added the setting of LAST_HTTP_CONTENT_RECEIVED_KEY for read specific case when there is a GET request and data is read , this will not impact PUT request where request streams are read even after receiving Last byte as part of put request

Testing

Tested by sending a huge object in S3Asycn.putObject and then disconnecting from VPN in between transfer. Expected outcome is it throws TimeOut Exceptions rather than waiting infinitely.
Added J units for this case by mocking server disconnects when transfer is in progress.

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

cenedhryn · 2022-11-03T17:17:53Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/internal/nrs/HttpStreamsHandler.java

@@ -211,7 +213,7 @@ private void handleCancelled(ChannelHandlerContext ctx, InT msg) {
    private void handleReadHttpContent(ChannelHandlerContext ctx, HttpContent content) {
        if (!ignoreBodyRead) {
            if (content instanceof LastHttpContent) {
-
+                ctx.channel().attr(LAST_HTTP_CONTENT_RECEIVED_KEY).set(true);


This is a nice simplification of the logic.

Do we want to move the logging statement over too?

done, added Debug log same as before

…set while uploading of objects

zoewangg · 2022-11-03T20:06:11Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/internal/nrs/HttpStreamsHandler.java

@@ -211,7 +215,8 @@ private void handleCancelled(ChannelHandlerContext ctx, InT msg) {
    private void handleReadHttpContent(ChannelHandlerContext ctx, HttpContent content) {
        if (!ignoreBodyRead) {
            if (content instanceof LastHttpContent) {
-
+                ctx.channel().attr(LAST_HTTP_CONTENT_RECEIVED_KEY).set(true);


Do we know in what situations ignoreBodyRead is set to true?

ignoreBodyRead is true when we get 200 OK and there is nothing to read

Example

ignoreBodyRead in False state while handleReadHttpContent

In the case of GetObject

We get 200 OK

Then we read the data

Last byte received

Now since get has data to read the ignoreBodyRead is in false state because this fails.

When set to true

In the case of PutObject

We get 200 OK

Then we SEND the data, nothing to read from service

Last byte received

Now since Put has no data to read the ignoreBodyRead is in true state because this passes.

It makes sense to set the LastByte flag true irrespective of ignoreBodyRead status

zoewangg · 2022-11-03T20:08:24Z

...client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelAttributeKey.java

+    /**
+     * {@link AttributeKey} to keep track of whether we have received the {@link LastHttpContent}.
+     */
+    public static final AttributeKey<Boolean> LAST_HTTP_CONTENT_RECEIVED_KEY = NettyUtils.getOrCreateAttributeKey(


Should we rename it to STREAMING_COMPLETE_KEY? Alternatively, we can update the description of this field to make it clear that it indicates streaming has finished

zoewangg · 2022-11-03T20:10:44Z

...t/src/test/java/software/amazon/awssdk/http/nio/netty/fault/SdkTestHttpContentPublisher.java

+import org.reactivestreams.Subscription;
+import software.amazon.awssdk.http.async.SdkHttpContentPublisher;
+
+public class SdkTestHttpContentPublisher implements SdkHttpContentPublisher {


Can we use the existing SimpleHttpContentPublisher or something from RxJava?

SimpleHttpContentPublisher is in core and netty didnot have core dependency, I did not find anything from RxJava, will keep looking out for it.

Added SimpleHttpContentPublisher in http-client-tests since adding in test-utils is casuing cyclic dependency.

…E_KEY and setting it irrespective of ignoreBodyRead flag

zoewangg · 2022-11-14T18:35:55Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/internal/nrs/HttpStreamsHandler.java

-            if (content instanceof LastHttpContent) {
-
+            if (lastHttpContent) {
+                logger.debug(ctx.channel(), () -> "Received LastHttpContent " + ctx.channel());


Maybe moving this log statement to line 218?

…dyRead flag

sonarqubecloud · 2022-11-14T19:57:34Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

64.7% Coverage
5.0% Duplication

…5b148a627 Pull request: release <- staging/42c674bc-70ed-49a8-9013-a745b148a627

joviegas requested a review from a team as a code owner November 3, 2022 05:20

joviegas mentioned this pull request Nov 3, 2022

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset while uploading of objects #3522

Closed

12 tasks

joviegas force-pushed the feature/master/joviegas_async_hang_bug branch from f30a6e2 to 8d076f8 Compare November 3, 2022 16:28

cenedhryn reviewed Nov 3, 2022

View reviewed changes

joviegas force-pushed the feature/master/joviegas_async_hang_bug branch from 8d076f8 to fd1287e Compare November 3, 2022 17:47

Bug fix for S3AsyncClient.putObject hangs if there is a connection re…

267eee9

…set while uploading of objects

joviegas force-pushed the feature/master/joviegas_async_hang_bug branch from fd1287e to 267eee9 Compare November 3, 2022 17:49

zoewangg reviewed Nov 3, 2022

View reviewed changes

joviegas force-pushed the feature/master/joviegas_async_hang_bug branch from 0c1e18b to be709d2 Compare November 4, 2022 00:24

Changing LAST_HTTP_CONTENT_RECEIVED_KEY key name to STREAMING_COMPLET…

e4af9ba

…E_KEY and setting it irrespective of ignoreBodyRead flag

joviegas force-pushed the feature/master/joviegas_async_hang_bug branch from be709d2 to e4af9ba Compare November 4, 2022 18:52

Added SimpleHttpContentPublisher and removed SdkTestHttpContentPublisher

64fc4e7

zoewangg approved these changes Nov 14, 2022

View reviewed changes

Moving log in handleReadHttpContent() to log irrespective of ignoreBo…

2121b1a

…dyRead flag

joviegas merged commit 876adfd into master Nov 15, 2022

joviegas deleted the feature/master/joviegas_async_hang_bug branch December 20, 2022 00:38

aws-sdk-java-automation added a commit that referenced this pull request Jan 2, 2025

Merge pull request #3535 from aws/staging/42c674bc-70ed-49a8-9013-a74…

1997538

…5b148a627 Pull request: release <- staging/42c674bc-70ed-49a8-9013-a745b148a627

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset set while uploading of object #3535

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset set while uploading of object #3535

Uh oh!

joviegas commented Nov 3, 2022

Uh oh!

cenedhryn Nov 3, 2022 •

edited

Loading

Uh oh!

joviegas Nov 3, 2022

Uh oh!

zoewangg Nov 3, 2022

Uh oh!

joviegas Nov 3, 2022 •

edited

Loading

Uh oh!

zoewangg Nov 3, 2022 •

edited

Loading

Uh oh!

joviegas Nov 4, 2022

Uh oh!

zoewangg Nov 3, 2022

Uh oh!

joviegas Nov 4, 2022

Uh oh!

joviegas Nov 4, 2022

Uh oh!

zoewangg Nov 14, 2022

Uh oh!

joviegas Nov 14, 2022

Uh oh!

sonarqubecloud bot commented Nov 14, 2022

Uh oh!

Uh oh!

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset set while uploading of object #3535

Bug fix for S3AsyncClient.putObject hangs if there is a connection reset set while uploading of object #3535

Uh oh!

Conversation

joviegas commented Nov 3, 2022

Motivation and Context

Root Cause

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

cenedhryn Nov 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

ignoreBodyRead in False state while handleReadHttpContent

When set to true

Uh oh!

zoewangg Nov 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 14, 2022

Uh oh!

Uh oh!

cenedhryn Nov 3, 2022 •

edited

Loading

joviegas Nov 3, 2022 •

edited

Loading

zoewangg Nov 3, 2022 •

edited

Loading