#857 Support multipart files using InputStream from source file #1593

samridh90 · 2018-10-26T05:38:45Z

Motivation:

In certain situations it's useful for the user to be able to provide an InputStream to a file instead of the File object itself for a multipart file part.
E.g.: Upload large files accessible over a network protocol such as SFTP to a remote server without copying the file to the local filesystem or loading it fully into memory

Changes:

Introduce new InputStreamParttype for use in the addBodyPart(...) API
Implement InputStreamMultipartPart to transfer InputStream to a Netty ByteBuf or WritableByteChannel
Unfortunately did not find a way to implement protected long transferContentTo(WritableByteChannel target) throws IOException for zero-copy uploads.
Added new tests and updated existing tests to cover scenarios similar to FilePart

Results:

It is now possible to provide an InputStream as a multipart body part

Related Issue:

#857

zbum · 2018-10-26T08:39:34Z

👍

slandelle

Thanks!

Could you please address comments?

slandelle · 2018-10-26T13:35:36Z

client/src/main/java/org/asynchttpclient/request/body/multipart/InputStreamPart.java

+  private final InputStream inputStream;
+  private final long contentLength;
+
+  public InputStreamPart(String name, InputStream inputStream, long contentLength, String fileName) {


small nit: could you please add a constructir without contenLength that sets the value as -1? There's a chance most users won't know the stream length beforehand.

client/src/test/java/org/asynchttpclient/request/body/multipart/MultipartUploadTest.java

slandelle · 2018-10-26T13:37:21Z

client/src/test/java/org/asynchttpclient/request/body/multipart/MultipartUploadTest.java

+    try (AsyncHttpClient c = asyncHttpClient(config().setDisableZeroCopy(disableZeroCopy))) {
+      InputStream inputStream = new BufferedInputStream(new FileInputStream(file));
+      Request r = post("http://localhost" + ":" + port1 + "/upload")
+              .addBodyPart(new InputStreamPart("file", inputStream, file.length(), file.getName(), "text/plain", UTF_8)).build();


Could you please test with and without Content-Length prior knowledge?

- Added tests for all combinations of known/unknown contentLength and enabled/disabled zeroCopy for `InputStreamPart` - Fixed how `length()` is computed for `MultipartPart` - returns -1 if contentLength < 0 - Added condition in `NettyBodyBody` to use `BodyChunkedBody` for "zeroCopy" of `RandomAccessBody` if contentLength is -1

samridh90 · 2018-10-26T17:54:31Z

I ran into some trouble with the Netty HttpObjectEncoder class when I tried to get zero-copy with -1 contentLength to work. Here's what happens:

In org/asynchttpclient/netty/request/body/NettyBodyBody.java#write(final Channel channel, NettyResponseFuture<?> future) we set msg = new BodyFileRegion((RandomAccessBody) body);
In io/netty/netty-codec-http/4.1.30.Final/netty-codec-http-4.1.30.Final.jar!/io/netty/handler/codec/http/HttpObjectEncoder.class#encode(ChannelHandlerContext ctx, Object msg, List<Object> out) this causes us to hit the following block

            case 2:
                if (buf != null) {
                    out.add(buf);
                }

                this.encodeChunkedContent(ctx, msg, contentLength(msg), out);
                break;

Taking a look at the contentLength(msg) method, this returns -1 when msg matches FileRegion

private static long contentLength(Object msg) {
        if (msg instanceof HttpContent) {
            return (long)((HttpContent)msg).content().readableBytes();
        } else if (msg instanceof ByteBuf) {
            return (long)((ByteBuf)msg).readableBytes();
        } else if (msg instanceof FileRegion) {
            return ((FileRegion)msg).count();
        } else {
            throw new IllegalStateException("unexpected message type: " + StringUtil.simpleClassName(msg));
        }
    }

In encodeChunkedContent(ChannelHandlerContext ctx, Object msg, long contentLength, List<Object> out):

private void encodeChunkedContent(ChannelHandlerContext ctx, Object msg, long contentLength, List<Object> out) {
        ByteBuf buf;
        if (contentLength > 0L) {
            String lengthHex = Long.toHexString(contentLength);
            buf = ctx.alloc().buffer(lengthHex.length() + 2);
            buf.writeCharSequence(lengthHex, CharsetUtil.US_ASCII);
            ByteBufUtil.writeShortBE(buf, 3338);
            out.add(buf);
            out.add(encodeAndRetain(msg));
            out.add(CRLF_BUF.duplicate());
        }

        if (msg instanceof LastHttpContent) {
            HttpHeaders headers = ((LastHttpContent)msg).trailingHeaders();
            if (headers.isEmpty()) {
                out.add(ZERO_CRLF_CRLF_BUF.duplicate());
            } else {
                buf = ctx.alloc().buffer((int)this.trailersEncodedSizeAccumulator);
                ByteBufUtil.writeMediumBE(buf, 3149066);
                this.encodeHeaders(headers, buf);
                ByteBufUtil.writeShortBE(buf, 3338);
                this.trailersEncodedSizeAccumulator = 0.2F * (float)padSizeForAccumulation(buf.readableBytes()) + 0.8F * this.trailersEncodedSizeAccumulator;
                out.add(buf);
            }
        } else if (contentLength == 0L) {
            out.add(encodeAndRetain(msg));
        }
    }

We basically miss all the blocks since the encodeChunkedContent method doesn't handle contentLength < 0L.

@slandelle : Not sure if my solution is acceptable, I'd be open to better ideas.

…tStreamPart

slandelle · 2018-10-26T20:01:34Z

samridh90 I don't think it's worth trying to contribute chunked transfert encoding support for Netty's FileRegion. We can just disable bypass zero-copy when content-length < 0.
InputStream isn't zero-copy anyway, such it would only be suboptimal when sending a multipart that mixes a local file and an InputStream.
Then, don't bother implementing zero-copy for InputStreamPart as it shouldn't be used.

WDYT?

samridh90 · 2018-10-26T20:59:06Z

@slandelle : Sounds good to me. I'll get rid of the zero-copy for InputStreamPart.

Edit: Done, latest update removes zero-copy for InputStreamPart and adds tests that assert that it should not be used.

- Fix tests, add additional tests to cover various combinations

…StreamPart

slandelle · 2018-10-27T10:08:18Z

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

@@ -54,13 +54,7 @@ public void write(final Channel channel, NettyResponseFuture<?> future) {

    Object msg;
    if (body instanceof RandomAccessBody && !ChannelManager.isSslHandlerConfigured(channel.pipeline()) && !config.isDisableZeroCopy()) {


There was some misunderstanding. In my previous comment, I pointed to this line. If you check the content-length here, you can force to the BodyChunkedInput branch if it's < 0. Hence, There's no need to advice the user to disable zero-copy globally, it can be completely transparent.

slandelle · 2018-10-27T10:11:35Z

README.md

 * `StringPart`

+**NOTE**: `InputStreamPart` does not support zero-copy transfers. Explicitly disable zero-copy using `config.setDisableZeroCopy(true)` when one of the body parts is an `InputStreamPart`.


Please revert.
There was some misunderstanding. Please check my comment above: #1593 (comment).

There's no need to ask the user to explicitly disable zero-copy globally.
You can just check the content-length and go with BodyChunkedInput if it's < 0.

Thanks for clearing that up.
I've reverted the changes and added a check to BodyChunkedInput if contentLength 0

…th InputStreamPart" This reverts commit 0a1e490.

This reverts commit 7ab6f45.

client/src/main/java/org/asynchttpclient/request/body/multipart/InputStreamPart.java

.../src/main/java/org/asynchttpclient/request/body/multipart/part/InputStreamMultipartPart.java

slandelle · 2018-10-27T16:44:40Z

client/src/test/java/org/asynchttpclient/request/body/InputStreamPartLargeFileTest.java

+ * software distributed under the Apache License Version 2.0 is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the Apache License Version 2.0 for the specific language governing permissions and limitations there under.
+ */


Please fix header: Sonatype is no longer involved in this project and new code should be donated to AHC.

slandelle · 2018-10-27T16:45:25Z

One last nit and we'll be good :)

Thanks!

slandelle · 2018-10-27T19:02:51Z

Merged. Great job, thanks!

samridh90 · 2018-10-29T16:26:32Z

@slandelle: Happy to contribute! I'm not familiar with AHC's release cadence, when do you expect to include this feature in a release?

slandelle · 2018-10-29T21:50:43Z

2.6.0 should be available on maven central in 15 min.

857 Support multipart files using InputStream from source file

049ee8d

slandelle requested changes Oct 26, 2018

View reviewed changes

Add unknown-content-length coverage for large file uploads using Inpu…

806670f

…tStreamPart

puresam added 2 commits October 26, 2018 15:10

Remove zero-copy implementation for InputStreamPart

7ab6f45

- Fix tests, add additional tests to cover various combinations

Update README.md to document lack of support for zero-copy with Input…

0a1e490

…StreamPart

slandelle reviewed Oct 27, 2018

View reviewed changes

slandelle requested changes Oct 27, 2018

View reviewed changes

puresam added 3 commits October 27, 2018 09:37

Revert "Update README.md to document lack of support for zero-copy wi…

db29e79

…th InputStreamPart" This reverts commit 0a1e490.

Revert "Remove zero-copy implementation for InputStreamPart"

7f5c687

This reverts commit 7ab6f45.

Use BodyChunkedInput in contentLength < 0 in NettyBodyBody

ec1f32f

slandelle requested changes Oct 27, 2018

View reviewed changes

Add license header to new files

28b3cf8

slandelle merged commit 97b6192 into AsyncHttpClient:master Oct 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#857 Support multipart files using InputStream from source file #1593

#857 Support multipart files using InputStream from source file #1593

samridh90 commented Oct 26, 2018

zbum commented Oct 26, 2018

slandelle left a comment

slandelle Oct 26, 2018

slandelle Oct 26, 2018

samridh90 commented Oct 26, 2018 •

edited

Loading

slandelle commented Oct 26, 2018

samridh90 commented Oct 26, 2018 •

edited

Loading

slandelle Oct 27, 2018

slandelle Oct 27, 2018 •

edited

Loading

samridh90 Oct 27, 2018

slandelle Oct 27, 2018

samridh90 Oct 27, 2018

slandelle commented Oct 27, 2018 •

edited

Loading

slandelle commented Oct 27, 2018

samridh90 commented Oct 29, 2018

slandelle commented Oct 29, 2018

		@@ -54,13 +54,7 @@ public void write(final Channel channel, NettyResponseFuture<?> future) {

		Object msg;
		if (body instanceof RandomAccessBody && !ChannelManager.isSslHandlerConfigured(channel.pipeline()) && !config.isDisableZeroCopy()) {

		* `StringPart`

		NOTE: `InputStreamPart` does not support zero-copy transfers. Explicitly disable zero-copy using `config.setDisableZeroCopy(true)` when one of the body parts is an `InputStreamPart`.

#857 Support multipart files using InputStream from source file #1593

#857 Support multipart files using InputStream from source file #1593

Conversation

samridh90 commented Oct 26, 2018

zbum commented Oct 26, 2018

slandelle left a comment

Choose a reason for hiding this comment

slandelle Oct 26, 2018

Choose a reason for hiding this comment

slandelle Oct 26, 2018

Choose a reason for hiding this comment

samridh90 commented Oct 26, 2018 • edited Loading

slandelle commented Oct 26, 2018

samridh90 commented Oct 26, 2018 • edited Loading

slandelle Oct 27, 2018

Choose a reason for hiding this comment

slandelle Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

samridh90 Oct 27, 2018

Choose a reason for hiding this comment

slandelle Oct 27, 2018

Choose a reason for hiding this comment

samridh90 Oct 27, 2018

Choose a reason for hiding this comment

slandelle commented Oct 27, 2018 • edited Loading

slandelle commented Oct 27, 2018

samridh90 commented Oct 29, 2018

slandelle commented Oct 29, 2018

samridh90 commented Oct 26, 2018 •

edited

Loading

samridh90 commented Oct 26, 2018 •

edited

Loading

slandelle Oct 27, 2018 •

edited

Loading

slandelle commented Oct 27, 2018 •

edited

Loading