On 31/07/15 16:06, Simone Bordet wrote:
Hi,
On Fri, Jul 31, 2015 at 3:56 PM, Michael McMahon
<michael.x.mcma...@oracle.com> wrote:
Ok. Now, on the whole flow-control issue, here is my thinking.
First that isFast() method is a relic from a previous version and should not
have been there.
My thinking is that the body processing methods of the response
body processor should be simply allowed to block. That exerts back pressure
in HTTP/1 by the socket buffer filling up and TCP flow control kicking in.
For HTTP/2, the protocol's own buffering and window management functions
achieve the same
thing.
Sure, if an application use blocking APIs, backpressure it achieved.
But as a generic library designer, it's not possible to know if the
application will use blocking APIs. If they don't want to, they can't
use JDK's HttpClient.
That may be ok, but would be a shame.
I brought this up because it's an API design issue, not something that
you can change with a better implementation while keeping the same
APIs.
If the API supports an asynchronous mode, it's trivial to implement a
blocking mode, see for example:
https://github.com/eclipse/jetty.project/blob/master/jetty-client/src/main/java/org/eclipse/jetty/client/api/Response.java#L238-L255
For sending, we already control the rate of outgoing data by how regularly
the implementation pulls data from the processor.
But you may need to notify the producer asynchronously.
Imagine you have HttpClient1 downloading data from server1, and
HttpClient2 that must upload this data to server2.
How would you do this with the current APIs in a completely asynchronous way ?
Well, it needs co-operation between the producer and the consumer
obviously. But, the onResponseBodyChunk() method could write to a queue
and block if it reaches a certain size. The onRequestBodyChunk() would
read off the data and send it on. When the queue reaches a lower limit it
notifies the producer to continue. It's not asynchronous, but is possible
and I'm not sure we're aiming at this type of application.
Having said that, this is an area we planned to look at again, after the
initial putback.
Yes, the API is designed that responses are handled in two stages
response headers and then response body.
Using the asynchronous API it is easy to create a composition of
the two stages and have one CompletableFuture object complete
after both stages. Probably, the most useful will be CompletableFuture<T>
where T is the actual body type. Similar can be achieved with the blocking
API.
I'm not sure you got my point, I apologize if I did not understand your answer.
The issue is not about the 2 stages of a response.
It's about *request* and *response*, together as an exchange,
completeness, for which there is no API.
Bad things happens when the response is complete and you think the
request is completed too, but it's not.
I'm not saying it's impossible to code this with the current APIs, but
I wanted to raise the issue.
Example: client uploads a video of 500 GiB using a single mapped
buffer from a file over a slow connection;
server has a bug and throws a NPE while reading the first content byte;
the NPE is converted to a HTTP response 500, which is downloaded
immediately to the client;
the client receives the full response 500 with its body, but the
request is still uploading the file.
An application would be interested in knowing when the whole exchange
(request + response) is complete, not when the response alone is
complete.
An application may also want to apply timeout to the whole exchange
(see below re: timeouts).
I see what you mean. There seems to be two issues here:
1) notification of request complete, which we don't have
an explicit API for, but request processors can do something
about that. Regardless maybe it is worth making this more explicit
in the API.
2) cancellation of such requests. See below.
Cancellation is part of the CompletableFuture API
But it has the wrong semantic, if you refer to Future.cancel(boolean).
Future.cancel(boolean) does not have any effect on the underlying
operation (in this case if the request has already started).
but not in the blocking API currently. HTTP/2 should be
able to implement it by resetting streams.
See above it's a semantic problem.
If you implement it respecting the Future semantic, it's not the right
semantic for HttpClient, and viceversa.
Strangely, Future.cancel() doesn't specify any such restriction, but
CompletableFuture.cancel() does. In theory, one could sub-class
CompletableFuture and implement cancellation behavior that way,
but the intent does seem to be that cancellation should come from outside
and just be reported through this class. And considering the point above
about stopping runaway request sends then maybe an explicit
cancellation API would be useful.
I think the docs could use a simple example, but I thought
it seemed a useful addition.
eg builder.headers("Foo", "foovalue", "Bar", "barvalue).
The effect is additive. So, there can be multiple values
for any header name
How do you know if a string is a name or a value ?
builder.headers("Foo", "Bar", "Baz")
sets header Foo with values Bar and Baz, or sets header Foo with value
Bar, and header Baz with value... what ?
IMHO the API is confusing, I prefer just doing builder.header("Foo",
"foovalue").header("Bar", "barvalue").
the parameters have to be specified in pairs
with (header name first, then value) * n
== Timeouts
There are no definition of timeouts, including idle timeouts and
exchange timeouts ?
Timeouts are all handled by the CompletableFuture API
But again they have the wrong semantic, if you refer to
Future.get(long, TimeUnit).
The expiration of that timeout does not mean that the request is
cancelled or anything else.
It just means that that amount of time has passed.
You can happily write:
while (true)
{
try
{
future.get(1, SECONDS);
break;
}
catch (TimeoutException x)
{
// Not done yet, try again.
}
}
without implying any effect on the actual underlying operation.
This means that the timeout expires, but the request is not cancelled,
and if it is never responded, it will occupy resources forever.
Yes, this is true also. Good points.
Thanks!
Michael
