RFR 8184285: Buffer sizes of Flow based BodyProcessor API

[Tobias Thierer]

Hi Michael -

thanks for your work! The fact that BufferingProcessor
<http://cr.openjdk.java.net/~michaelm/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.
   - 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?
   - 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.
   - So far it looks like the new code will interoperate with blocking
   facades such as PipedResponseStream
   <http://cr.openjdk.java.net/~martin/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!

Tobias

On Thu, Aug 3, 2017 at 10:02 AM, Michael McMahon <
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/
>
> Thanks,
> Michael
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20170804/62001f9e/attachment.html>