Allow ByteBuffer.position() to be used for the data length.

[carloseltuerto]

Today, when sending the request body, the provided ByteBuffer is read entirely - its internal pointers such as the position and the limit are ignored. For the Cronvoy implementation, this is unfortunate. The lifecycle of the ByteBuffers used to send data are fully controlled by Cronet. Therefore, Cronet creates a ByteBuffer without knowing how many bytes the user will put in it. This can be made to work, but before sending the data, a new ByteBuffer needs to be created with the actual size that was put in the original ByteBuffer.

This PR attempts to solve this. By default, and this won't change, ByteBuffer.capacity() is used to determine the length when invoking EnvoyHTTPStream.sendData(). When configuring a new Stream, the user can now specify what should be used for the "length". StreamPrototype.setUseByteBufferPosition is the new method allowing to rather use the ByteBuffer.position() as the length. Then, for Cronet, no need to copy the ByteBuffer with this strategy.

Technical note: the general expectation with a ByteBuffer is to rely on its limit and its position when accessing the data. Relying solely on its capacity is somewhat unorthodox. The proposed implementation with this PR is a bit more in line with the ByteBuffer intended usage, but is not "by the book" either: Normally, when reading, the ByteBuffer must be "flipped", and then each time bytes are consumed, the position must be increased accordingly. This implementation leaves the ByteBuffer untouched when sending. Trying to perfectly mimic this behavior might be tricky: the C++ layer would need to update the ByteBuffer's position once some data has been consumed. For Cronet, this lack of conformity is not concerning: the ByteBuffer lifecycle is fully controlled by Cronet, not by the user - by contract, the user only puts the data it wants to send in the Cronet provided ByteBuffer, strictly when asks to do so (through callback), nothing else.

Description: Add a mechanism that avoids Cronet to copy ByteBuffers when sending the request body.
Risk Level: Small: the business logic is the same when the feature is not used, which is the default.
Testing: CI, Unit Test added
Docs Changes: N/A
Release Notes: N/A
Signed-off-by: Charles Le Borgne cleborgne@google.com

[goaway]

Looks great. I'd even be fine with dropping the option to use capacity and just use position by default, but it doesn't hurt to have the option to choose. Thanks @carloseltuerto!

[goaway]

Note: we actually do have a hook that we could use to "flip" the Java byte buffer - the C++ buffer API, will "drain" the buffer when it's done processing the data, and provides a callback we've already wired up to the bridge layer. Forwarding it up to Java to flip the passed ByteBuffer should be quite doable.

[junr03]

@carloseltuerto I believe we need to fix this. I am seeing malloc aborts thanks to data_length being obscenely high. For instance on a debug branch of the Lyft app I am seeing cases where the returned value here is 18446744073709551615

cc @goaway

[junr03]

We also need better testing in these utility functions. They influence a lot of other behavior. So even if the larger feature in this PR was opt in, changing the utility functions in the bridge layer has a wider impact.

[Reflejo]

So stating the obvious here but 18446744073709551615 is 2^64-1 which is -1. I think the bug here is that for some reason GetDirectBufferCapacity is returning -1? And one difference is we are now static casting to size_t. Only reason I can think that could happen is either j_data is not an initialized buffer or the JVM on that case for some reason doesn't support JNI access to buffers? The latter seems strange. Just theorizing.

[junr03]

So stating the obvious here but 18446744073709551615 is 2^64-1 which is -1

Wasn't so obvious to me when I was looking at this over the weekend 🤦. Thanks for having sharper eyes than mine @Reflejo.

@carloseltuerto we synced internally and @goaway is going to own helping with this.

[goaway]

I'm betting this is happening when the buffer is not a direct buffer.

[goaway]

Pretty sure I see the issue, will post a fix shortly.