RFR: 8087112: HTTP API and HTTP/1.1 implementation

[Michael McMahon]

On 28/09/15 10:13, Simone Bordet wrote:
> Hi,
>
> On Mon, Sep 28, 2015 at 10:45 AM, Michael McMahon
> <michael.x.mcmahon at oracle.com> wrote:
>> The API has already been approved and the first version to
>> be integrated won't have all suggestions incorporated.
>> But, it should be possible to address many of them in time
>> for JDK 9 anyway.
> Okay.
>
>>> A) lack of backpressure,
>> My current thoughts on this is to have credit based flow control
>> window (similar to the Flow API) indicating the number of bytes
>> that a HttpResponseBodyProcessor is willing to accept and this
>> maps directly on to the HTTP/2 flow control window and the socket
>> buffer when doing HTTP/1.
> Bear in mind that application must have control over this, so you have
> to expose an API that application can use to notify that they are done
> with the content.
> I am not sure the number of bytes is the right unit - my experience is
> that you want to trade ByteBuffers as a unit, not the bytes they
> contain.

if the units are ByteBuffers say, then how do you convert a notification
of n units into a HTTP/2 window update (which has to be in bytes)?

Perhaps, if n is required always to be equal to 1, then you could do it
because you would always be referring to the last buffer received
but it seems trickier if n > 1.

> This is also critical for buffer reuse and to avoid nasty surprises on
> who owns the buffer.
>
> To be clear:
>
> class MyResponseProcessor implements HttpResponseBodyProcessor
> {
>      void onResponseBodyChunk(ByteBuffer buffer)
>      {
>          writeAsyncToDisk(buffer, new CompletionHandler()
>          {
>              success: <tell implementation it can call onResponseBodyChunk again>
>              failure: <tell implementation to abort response>
>          });
>      }
> }
>
>>> B) missing "request complete" event
>> When you suggested this, I decided to split the API so that requests
>> and responses complete independently, but this was too drastic a change
>> and most use-cases are satisfied by completion of both request and response.
> I am not sure I follow here.
> There is currently no way for an application to be notified of request
> completion, so there is no way to be notified of completion of both
> request and response, only of response.
> Are you saying that most use cases will require this, and so it needs
> to be addressed, or are you saying that it is already done in a way
> that I could not see ?
> Can you expand on this ?

What I was saying was that completion of response also implies
competion of request (ie that it isn't notified until after both happens)

>> It's also possible to be notified of request completion through the
>> request processor. It should be straight forward to wrap any request
>> processor
>> in a processor which provides a CompletableFuture for completion of the
>> request.
> You mean HttpRequestBodyProcessor ?
> I don't think it's invoked if there is no body, and it has no
> onRequestBodyComplete() method.
> Can you expand again ?

It doesn't have a onComplete() type method. The notification
is implied by the final call to onRequestBodyChunk(). But, I accept
that doesn't actually mean the data has been fully written.
So, maybe an onComplete() method is useful ...
>>> C) missing timeouts (CompletableFuture.get(time) is obviously not the
>>> right semantic)
>> CompletableFuture.get() is at least useful for notifying timeouts.
> There is the need for a completely async timeout.
>
> Plus, the semantic of Future.get() is that the time you specified to
> get() has passed.
> There is no implied semantic that the request has been aborted, and
> aborting the request via cancel() would break the Future.get(time)
> contract.

Why would it break the contract? Note, I am referring to
HttpRequest.cancel() not CompletableFuture.cancel()

> Really, Future.get(time) is the wrong semantic, you don't want to use that.
>
> I suggest that blocking methods throw TimeoutException, and that async
> methods get notified with a TimeoutException on their
> CompletableFuture.
>
>> The question then is what to do next and what was missing was
>> cancellation, and that has been added. Does that not suffice?
> Nope. As said above, there is no async timeout, no idle timeout, no
> total (request + response) timeout, which are in my experience the 3
> most requested features about timeouts.
>