Added a way to define a body in ReactiveStreamBodies

[schmitch]

Actually this would be a approach for #1136

[slandelle]

@jroper Thought?

[jroper]

I think this makes sense, though it'd be good to see javadoc on NettyBody that describes how getContentLength is used - I'm assuming that if you return -1, that tells AHC to set the chunked headers and send a chunked body, otherwise it will set a content length. Similar javadocs mentioning that -1 (or not setting at all) will lead to chunked encoding could also be added to Request.getContentLength and RequestBuilderBase.setContentLength, I guess.

[slandelle]

I think this makes sense, though it'd be good to see javadoc on NettyBody that describes how getContentLength is used - I'm assuming that if you return -1, that tells AHC to set the chunked headers and send a chunked body, otherwise it will set a content length.

Agree, but on Body (the API), not NettyBody (impl). :)

Similar javadocs mentioning that -1 (or not setting at all) will lead to chunked encoding could also be added to Request.getContentLength and RequestBuilderBase.setContentLength, I guess.

Actually, those were only used by the Grizzly and the JDK provider in 1.9. I missed that I should remove them in AHC2.

Thanks!

[slandelle]

Shoot, I didn't realize this PR was indeed introducing a request.getContentLength usage.
I'm going to revert the merge.

Let's then discuss how to bring a Content-Length there. It should be the Body's responsibility.

[schmitch]

Wouldn't my take won't be good enough? Or how do you mean "it should be the Body's responsibility". I mean it's a streamed response, but maybe the user already know the Content-Length and wants to set it btw. we could discuss in the issue I've created #1136 and I try my best to do it.

The last night I just wanted a quick demo so i was very sleepy when I did it.

Edit: Oh I see you actually removed the method.

Another way of doing that would actually be using the Header and extracting it. As said the Content-Length should be set manually.

Use case would be using the Streaming interface to upload multipart/form-data via a Publisher where the other endpoint only supports Http 1.0

[slandelle]

Wouldn't my take won't be good enough?

No. As of #1138, I just remove Request.getContentLength, and RequestBuidler.setContentLength. Those where dead code that were used by the Grizzly provider in AHC1.

I mean it's a streamed response, but maybe the user already know the Content-Length and wants to set it btw.

Don't get me wrong, I agree with the feature (being able to set the Content-Length).
Not with the implementation (passed from the Request/RequestBuilder).

It would be confusing to have such methods in the Generic API that are only used from for ReactiveStreams.

[schmitch]

@slandelle see my edit, what about:
https://github.com/AsyncHttpClient/async-http-client/blob/master/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestFactory.java#L169

Changing this part to check if there is a existing Header? i.e. either TRANSFER_ENCODING or CONTENT_LENGTH and keep it if it's there ?
That would mean that all Bodies would honor it.

[slandelle]

That looks like a very complicated way to me.

IMO, the proper solution would be to overload RequestBuilderBase#setBody(Publisher<ByteBuffer>) and introduce a version that takes a Content-Length and pass it to the ReactiveStreamsBodyGenerator.

[schmitch]

That would be fine to me too however that would mean we would need to change play-ws while the Header change wouldn't need a change. I open a new PR with your solution.

[slandelle]

that would mean we would need to change play-ws while the Header change wouldn't need a change.

Well, from my pov, that's a new feature. :)

[schmitch]

#1139 opened, suggestions are welcomed so that I could fix it when I'm done with work. ;)