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.mcma...@oracle.com <mailto:michael.x.mcma...@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
