Workaround for netty chunked upload issue #1563

beargummy · 2018-07-18T14:29:43Z

Motivation:
Netty doesn't provide any ability to handle early server response when uploading chunked file and instead, IOException: Pipe closed is thrown (see netty/netty#6706 for details).

Changes:
NettyInputStreamBody swallows ChannelOutputShutdownException in case it is thrown from netty's ChunkedWriteHandler.

Result:
Multipart early response is available to be handled properly.

slandelle · 2018-07-18T15:04:42Z

client/src/main/java/org/asynchttpclient/netty/future/StackTraceInspector.java

+          String className = element.getClassName();
+          String methodName = element.getMethodName();
+          if (className.contains("ChunkedWriteHandler") && (methodName.equals("flush") || methodName.equals("write")))
+            System.exit(1);


lol... sorry

anyway build will fail due to some issues:

Dynamic Oracle JDK installations are currently unavailable due to a problem with our upstream provider. In the meantime, container-based builds come with JDKs preinstalled.

beargummy · 2018-07-18T15:14:40Z

The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $TRAVIS_APT_OPTS install oracle-java8-installer" failed and exited with 100 during .

not my day :D

slandelle · 2018-07-20T06:33:23Z

@beargummy Any reason why you closed this PR? Is it broken?

beargummy · 2018-07-20T11:45:02Z

I found one more case which also affected by this issue (BodyChunkedInput)

slandelle · 2018-07-23T10:30:45Z

client/src/main/java/org/asynchttpclient/netty/future/StackTraceInspector.java

+          if (className.contains("ChunkedWriteHandler") && (methodName.equals("flush") || methodName.equals("write")))
+            return true;
+        }
+      } catch (Throwable ignore) {


What's this try/catch for? Are you sure you want to catch Throwable???

slandelle · 2018-07-23T10:35:07Z

client/src/main/java/org/asynchttpclient/netty/request/body/NettyBodyBody.java

+            .addListener(new WriteProgressListener(future, false, getContentLength()) {
+              @Override
+              public void operationComplete(ChannelProgressiveFuture cf) {
+                closeSilently(body);


Why do you need that? I would expect Netty to close the ChunkedInput.

slandelle · 2018-07-23T10:35:38Z

client/src/main/java/org/asynchttpclient/netty/request/body/NettyBodyBody.java

@@ -56,6 +59,14 @@ public void write(final Channel channel, NettyResponseFuture<?> future) {
    if (body instanceof RandomAccessBody && !ChannelManager.isSslHandlerConfigured(channel.pipeline()) && !config.isDisableZeroCopy()) {
      msg = new BodyFileRegion((RandomAccessBody) body);


Is there any reason you only fixed for ChunkedInput and not for FileRegion?

I haven't seen it failing, but I'll add workaround for this case too.

slandelle · 2018-07-23T10:37:22Z

client/src/main/java/org/asynchttpclient/netty/future/StackTraceInspector.java

+      }
+    }
+
+    return t.getCause() != null && recoverOnChunkedUploadFailed(t.getCause());


Did you check which Exception you actually get? Do you really need to recurse?

You're right, there is no need for recursive call.

slandelle · 2018-07-23T10:38:30Z

client/src/main/java/org/asynchttpclient/netty/request/body/NettyBodyBody.java

+                // FIXME dirty hack until netty issue is not resolved, see https://github.com/netty/netty/issues/6706
+                if (cause instanceof IOException && StackTraceInspector.recoverOnChunkedUploadFailed(cause))
+                  return false;
+                return super.abortOnThrowable(channel, cause);


merge into one single boolean op

slandelle · 2018-07-23T10:38:36Z

client/src/main/java/org/asynchttpclient/netty/request/body/NettyInputStreamBody.java

+                // FIXME dirty hack until netty issue is not resolved, see https://github.com/netty/netty/issues/6706
+                if (cause instanceof IOException && StackTraceInspector.recoverOnChunkedUploadFailed(cause))
+                  return false;
+                return super.abortOnThrowable(channel, cause);


merge into one single boolean op

Motivation: Netty doesn't provide any ability to handle early server response when uploading chunked file and instead, `IOException: Pipe closed` is thrown (see netty/netty#6706 for details). Changes: NettyInputStreamBody and BodyChunkedInput swallow ChannelOutputShutdownException in case it is thrown from netty's ChunkedWriteHandler. Result: Multipart early response is available to be handled properly.

beargummy · 2018-07-24T08:27:50Z

@slandelle do you have any objections?

slandelle · 2018-07-24T08:56:30Z

client/src/main/java/org/asynchttpclient/netty/request/body/NettyBodyBody.java

+              @Override
+              protected boolean abortOnThrowable(Channel channel, Throwable cause) {
+                // FIXME dirty hack until netty issue is not resolved, see https://github.com/netty/netty/issues/6706
+                return StackTraceInspector.recoverOnChunkedUploadFailed(cause)


Actually, do we really need stacktrace inspection?
We already know that we're uploading a body. Is there really a way to get a ChannelOutputShutdownException whose stacktrace wouldn't pass though ChunkedWriteHandler#flush (note that there's still some work to do with FileRegion)?

Then, my concern is that I'm not sure wether a ChannelOutputShutdownException always means that we'll still be able to read nor if there's anything to read at all. I'm afraid if we don't have a response to read, we'll hang in half closed state forever.

Yes, you're right, there is no need to check stack trace in this case.
Regarding FileRegion - it is not affected by this issue, since ChunkedWriteHandler handles it in totally different manner.

I'm pretty sure there's the same issue with FileRegion, but that it can't be fixed the same way :(

Well, I wasn't able to reproduce the issue for FileRegion so far. But maybe you're right.

slandelle · 2018-07-24T09:03:02Z

client/src/test/java/org/asynchttpclient/test/EchoHandler.java

@@ -69,6 +69,13 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt
        httpResponse.sendRedirect(httpRequest.getHeader("X-redirect"));
        return;
      }
+      if (headerName.startsWith("X-fail")) {
+        httpResponse.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
+        httpResponse.getOutputStream().write("custom error message".getBytes());


This response is invalid: as you set a body (and we want one as we want to test this use case), you need to set the Content-Length.

String#getBytes is something horrible that should be dropped from Java. Please pass the Charset, eg getBytes(StandardCharsets.US_ASCII/UTF_8)

slandelle · 2018-07-24T09:05:54Z

client/src/test/java/org/asynchttpclient/test/EchoHandler.java

@@ -69,6 +69,13 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt
        httpResponse.sendRedirect(httpRequest.getHeader("X-redirect"));
        return;
      }
+      if (headerName.startsWith("X-fail")) {
+        httpResponse.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
+        httpResponse.getOutputStream().write("custom error message".getBytes());


String#getBytes is something horrible that should be dropped from Java. Please pass the Charset, eg getBytes(StandardCharsets.US_ASCII/UTF_8)

slandelle reviewed Jul 18, 2018

View reviewed changes

beargummy force-pushed the netty-chunked-upload-workaround branch from c3e81d0 to 3cf0473 Compare July 18, 2018 15:11

beargummy closed this Jul 19, 2018

beargummy reopened this Jul 20, 2018

slandelle requested changes Jul 23, 2018

View reviewed changes

beargummy force-pushed the netty-chunked-upload-workaround branch from df27f2d to cc71b72 Compare July 23, 2018 13:27

slandelle reviewed Jul 24, 2018

View reviewed changes

slandelle requested changes Jul 24, 2018

View reviewed changes

Daniil Kudryavtsev added 2 commits July 24, 2018 19:36

nit

4fbc64e

Remove unnecessary stack trace check

7689092

kachayev mentioned this pull request Dec 19, 2018

Premature server response causes HTTP client to crash clj-commons/aleph#454

Open

beargummy closed this Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for netty chunked upload issue #1563

Workaround for netty chunked upload issue #1563

beargummy commented Jul 18, 2018

slandelle Jul 18, 2018

beargummy Jul 18, 2018

beargummy Jul 18, 2018

beargummy commented Jul 18, 2018

slandelle commented Jul 20, 2018

beargummy commented Jul 20, 2018

slandelle Jul 23, 2018

slandelle Jul 23, 2018

slandelle Jul 23, 2018

beargummy Jul 23, 2018

slandelle Jul 23, 2018

beargummy Jul 23, 2018

slandelle Jul 23, 2018

slandelle Jul 23, 2018

beargummy commented Jul 24, 2018

slandelle Jul 24, 2018 •

edited

Loading

beargummy Jul 25, 2018

slandelle Jul 25, 2018

beargummy Jul 25, 2018

slandelle Jul 24, 2018

slandelle Jul 24, 2018

slandelle Jul 24, 2018

		@@ -56,6 +59,14 @@ public void write(final Channel channel, NettyResponseFuture<?> future) {
		if (body instanceof RandomAccessBody && !ChannelManager.isSslHandlerConfigured(channel.pipeline()) && !config.isDisableZeroCopy()) {
		msg = new BodyFileRegion((RandomAccessBody) body);

Workaround for netty chunked upload issue #1563

Workaround for netty chunked upload issue #1563

Conversation

beargummy commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beargummy commented Jul 18, 2018

slandelle commented Jul 20, 2018

beargummy commented Jul 20, 2018

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

beargummy commented Jul 24, 2018

slandelle Jul 24, 2018 • edited Loading

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

slandelle Jul 24, 2018 •

edited

Loading