RFR 8184285: Buffer sizes of Flow based BodyProcessor API

[Michael McMahon]

Hi Tobias,

On 04/08/2017, 10:22, Tobias Thierer wrote:
> Hi Michael -
>
> thanks for your work! The fact that BufferingProcessor 
> <http://cr.openjdk.java.net/%7Emichaelm/8184285/webrev.1/src/jdk.incubator.httpclient/share/classes/jdk/incubator/http/BufferingProcessor.java.html> wraps 
> another BodyProcessor looks like it could be quite useful/flexible.
>
> Some things I noticed upon a quick glance:
>
>   * BufferingProcessor's documentation cuts out mid-sentence.
>
That could be just how the unformatted text looks. I've uploaded the 
HTML apidoc
for the two interfaces to:
http://cr.openjdk.java.net/~michaelm/8184285/apidoc/
which should be easier to read. Links won't work but you can just search 
for the buffering() method in both docs.
>
>   * BufferingProcessor passes modifiable LinkedList instances. Why? Do
>     you want to guarantee that remove-from-front is efficient? If not,
>     consider singleton and array-based unmodifiable instances since
>     they tend to be faster, more memory efficient, and don't risk
>     applications starting to accidentally rely on the lists being
>     modifiable?
>
That seems like a good idea.
>
>   * Unsurprisingly, getBuffersOf() performs a copy via getNBytesFrom()
>     - just a thing to be aware of, but I don't expect it can be avoided.
>   * The buffer size is fixed, so one can't change the buffer size
>     dynamically (e.g. in response to bitrate changes in a video player
>     app) or even make the first buffer a different size than the later
>     ones (e.g. file header vs. later chunks). I don't know a good
>     solution to the latter either, since Flow.request(long) documents
>     that the long refers to the number of calls; the former looks like
>     it could be made more flexible in a future version of
>     BufferingProcessor, but is probably okay for now.
>
At the very least it would require defining a new subtype of 
HttpResponse.BufferingProcessor and it complicates the Flow semantics
a bit, because you could be changing the buffersize while there is 
outstanding demand at the old buffersize. But, it could be usable
if you stick to doing request(1) all the time. I think we should defer 
on that for now though.
>
>   * So far it looks like the new code will interoperate with blocking
>     facades such as PipedResponseStream
>     <http://cr.openjdk.java.net/%7Emartin/http-client/src/com/google/test/http/PipedResponseStream.java>,
>     which is nice. I hope to verify this empirically next week.
>
> I intend to write more comments later, just wanted to send a quick 
> first impression. Thanks for the promising update!
>
Great thanks for the first impression.

Michael
> Tobias
>
> On Thu, Aug 3, 2017 at 10:02 AM, Michael McMahon 
> <michael.x.mcmahon at oracle.com <mailto:michael.x.mcmahon at oracle.com>> 
> wrote:
>
>     Hi,
>
>     The HTTP client work is continuing in a new branch of the JDK 10
>     sandbox forest (http-client-branch),
>     and here is the first of a number of changes we want to make.
>
>     This one is to address the feedback we received where
>     HttpResponse.BodyProcessors would
>     be easier to implement if there was control over the size of
>     buffers being supplied.
>
>     To that end we have added APIs for creating buffered response
>     processors (and handlers)
>
>     So, HttpResponse.BodyProcessor has a new static method with the
>     following signature
>
>     public static <T> BodyProcessor<T> buffering(BodyProcessor<T>
>     downstream, long buffersize) {}
>
>     This returns a new processor which delivers data to the supplied
>     downstream processor, buffered
>     by the 'buffersize' parameter. It guarantees that all data is
>     delivered in chunks of that size
>     until the final chunk, which may be smaller.
>
>     This should allow other BodyProcessor implementations that require
>     buffering to wrap themselves
>     in this way, be guaranteed that the data they receive is buffered,
>     and then return that composite
>     processor to their user.
>
>     A similar method is added to HttpResponse.BodyHandler.
>
>     Note also, that we have changed HttpResponse.BodyProcessor from
>     being a Flow.Subscriber<ByteBuffer>
>     to Flow.Subscriber(List<ByteBuffer>). That change is technically
>     orthogonal to this one, but is motivated
>     by it. By transferring ByteBuffers in lists makes it easier to
>     buffer them efficiently.
>
>     The webrev is at:
>     http://cr.openjdk.java.net/~michaelm/8184285/webrev.1/
>     <http://cr.openjdk.java.net/%7Emichaelm/8184285/webrev.1/>
>
>     Thanks,
>     Michael
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20170804/621ef15a/attachment.html>