Netty async stability fixes #173

dagnir · 2017-09-21T00:20:00Z

Fix data corruption bug in streaming responses
Fix failure to signal the SdkResponseHandler in situations where remote end of the TCP connection is unexpectedly closed by the service

Builds on @shorea's work here: https://github.com/aws/aws-sdk-java-v2/tree/shorea-dongie-perf-share

Fixes #146

- Prevent possible use-after-free bug for Streaming responses - Account for half closed connection (closed by remote) and signal back to SdkResponseHandler asn an IOException Fixes aws#146

shorea

Nice!

shorea · 2017-09-21T15:17:16Z

http-clients/netty-nio-client/pom.xml

@@ -60,6 +60,14 @@

        <!--Reactive Dependencies-->
        <dependency>
+            <groupId>io.netty</groupId>


You can probably remove this. Was testing the perf of native SSL.

shorea · 2017-09-21T15:17:31Z

...-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java

@@ -143,7 +145,7 @@ private static int port(URI uri) {

    private SslContext sslContext(String scheme) {
        if (scheme.equalsIgnoreCase("https")) {
-            SslContextBuilder builder = SslContextBuilder.forClient().sslProvider(defaultClientProvider());
+            SslContextBuilder builder = SslContextBuilder.forClient().sslProvider(SslProvider.JDK);


This you can revert for now.

shorea · 2017-09-21T15:19:07Z

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

-        ctx.channel().close()
-           .addListener(channelFuture -> requestContext.channelPool().release(ctx.channel()));
+    private static void removePerRequestHandlers(ChannelHandlerContext ctx) {
+        removePerRequestHandlers(ctx, HttpStreamsClientHandler.class, ReadTimeoutHandler.class, WriteTimeoutHandler.class);


I think we can get rid of this and remove them before the request starts.

+1 looks like we were already doing that; removed

shorea · 2017-09-21T15:19:38Z

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

+            ByteBuf fullContent = ((FullHttpResponse) msg).content();
+            final ByteBuffer bb = copyToByteBuffer(fullContent);
+            fullContent.release();
+            requestContext.handler().onStream(new FullResponseContentPublisher(channelContext, bb));
        }

        if (msg instanceof LastHttpContent) {


If you remove the removePerRequsetHandlers method I think you can combine this with the FullHttpResponse branch.

shorea · 2017-09-21T15:27:22Z

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

        }
    }

    @Override
    public void abort() {
        if (channel != null) {
-            channel.disconnect().addListener(ignored -> context.channelPool().release(channel));
+            channel.close().addListener(ignored -> context.channelPool().release(channel));


a closeAndRelease method would be good here too.

Refactored into closeAndRelease

spfink

Any helpful comments we could add to the SPI based on the changes you had to make here?

spfink · 2017-09-21T16:43:13Z

bundle/pom.xml

@@ -54,6 +54,10 @@
                <configuration>
                    <artifactSet>
                        <includes>
+                            <include>joda-time:joda-time</include>


Where are we still using joda?

Good catch! Removed

spfink · 2017-09-21T16:43:44Z

core/src/main/java/software/amazon/awssdk/async/FileAsyncRequestProvider.java

                }
+            } catch (Exception e) {
+                subscriber.onError(e);


Comment or logging here?

spfink · 2017-09-21T16:44:16Z

core/src/main/java/software/amazon/awssdk/async/FileAsyncResponseHandler.java

+            }
+        }
+
+        private void close() {
            // Completion handled by response handler


No longer true?

Looks like this got left it while I was resolving some merge conflicts

spfink · 2017-09-21T16:44:58Z

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

-                                                             .statusCode(response.status().code())
-                                                             .statusText(response.status().reasonPhrase())
-                                                             .build();
+                    .headers(fromNettyHeaders(response.headers()))


Oh man, are we fixed or aligned?

Haha, realigned this.

shorea · 2017-09-25T16:42:27Z

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

+            ByteBuf fullContent = ((FullHttpResponse) msg).content();
+            final ByteBuffer bb = copyToByteBuffer(fullContent);
+            fullContent.release();
+            requestContext.handler().onStream(new FullResponseContentPublisher(channelContext, bb));
            Subscriber<? super ByteBuffer> subscriber = channelContext.channel().attr(ChannelAttributeKeys.SUBSCRIBER_KEY).get();


Do we still need to stick the subscriber in an attribute if we complete on the same message?

Yeah since the FullResponseContentPublisher is the one that sets the subscriber in the ChannelContext

shorea · 2017-09-25T16:43:46Z

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

+    public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
+        RequestContext requestContext = ctx.channel().attr(REQUEST_CONTEXT_KEY).get();
+        log.error("Exception processing request: {}", requestContext.sdkRequest(), cause);
+        runAndLogError("SdkHttpResponseHandler threw an exception",


Should we mark complete here too?

shorea · 2017-09-25T16:48:38Z

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

                        requestContext.handler().complete();
                    } finally {
-                        finalizeRequest(requestContext, channelContext);
+                        // Run in the event loop to avoid sync issues in finalizeRequest


I think we should be guaranteed to be running on the channel thread.

Double checked this and looks like HttpStreamsClientHandler does guarantee this. Thanks for pointing it out!

shorea

LGTM! Nice work!

shorea · 2017-09-26T17:19:59Z

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


+        // TODO make these configurable


Can you modify this TODO to only add the write here, remove it when we finished writing, add the read then, then remove read when the request if finished?

shorea and others added 2 commits September 20, 2017 16:00

Various performance and stability improvements for NIO.

f31987a

Fixes for netty async streaming requests/responses

6468614

- Prevent possible use-after-free bug for Streaming responses - Account for half closed connection (closed by remote) and signal back to SdkResponseHandler asn an IOException Fixes aws#146

shorea reviewed Sep 21, 2017

View reviewed changes

spfink reviewed Sep 21, 2017

View reviewed changes

dagnir force-pushed the async-fixes-final branch from a559dc1 to 10927ca Compare September 22, 2017 00:51

shorea reviewed Sep 25, 2017

View reviewed changes

dagnir force-pushed the async-fixes-final branch from 10927ca to 6992171 Compare September 25, 2017 17:07

shorea approved these changes Sep 26, 2017

View reviewed changes

Cleanup unused files, code changes

bc7060d

dagnir force-pushed the async-fixes-final branch from 6992171 to bc7060d Compare September 26, 2017 17:31

Merge branch 'master' into async-fixes-final

3da471e

dagnir merged commit f4d3685 into aws:master Sep 26, 2017

This was referenced Oct 17, 2017

Preview 5 change log #213

Merged

WIP #228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netty async stability fixes #173

Netty async stability fixes #173

dagnir commented Sep 21, 2017

shorea left a comment

shorea Sep 21, 2017

dagnir Sep 22, 2017

shorea Sep 21, 2017

dagnir Sep 22, 2017

shorea Sep 21, 2017

dagnir Sep 22, 2017

shorea Sep 21, 2017

shorea Sep 21, 2017

dagnir Sep 22, 2017

spfink left a comment

spfink Sep 21, 2017

dagnir Sep 22, 2017

spfink Sep 21, 2017

spfink Sep 21, 2017

dagnir Sep 22, 2017

spfink Sep 21, 2017

dagnir Sep 22, 2017

shorea Sep 25, 2017

dagnir Sep 25, 2017

shorea Sep 25, 2017

dagnir Sep 25, 2017

shorea Sep 25, 2017

dagnir Sep 25, 2017

shorea left a comment

shorea Sep 26, 2017

dagnir Sep 26, 2017

Netty async stability fixes #173

Netty async stability fixes #173

Conversation

dagnir commented Sep 21, 2017

shorea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spfink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shorea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment