We’ve almost come to agreement on this ( at least as much as is possible
without further prototyping ). The point where we reached, which I think is
a good solution, leaves connect timeout and response timeout as
orthogonal mechanisms. I would like to separate the issues.
1) 8208391 [1] will be used to implement and support a connect timeout
as discussed below. I’ll send a separate review request for this.
2) 8208693 [2] will track the issue around the existing timeout and
the possibility of extending the current implementation to cover the
request+body AND response+body
-Chris.
[1] https://bugs.openjdk.java.net/browse/JDK-8208391
[2]https://bugs.openjdk.java.net/browse/JDK-8208693
> On 1 Aug 2018, at 20:49, Chris Hegarty <chris.hega...@oracle.com> wrote:
>
> Simone,
>
>> On 1 Aug 2018, at 20:20, Simone Bordet <simone.bor...@gmail.com> wrote:
>>
>> Hi,
>>
>> On Wed, Aug 1, 2018 at 5:10 PM Chris Hegarty <chris.hega...@oracle.com>
>> wrote:
>>> Before trying to draft the javadoc, the semantic we want for this
>>> overall timeout is that it should approximately related to the observed
>>> time spent in send[Async].
>>
>> I'm not sure I agree, especially for sendAsync(), which should setup
>> things for sending and then immediately return.
>
> The CompletableFuture returned by sendAsync must complete
> exceptionally with an HttpTimeoutException, say if the response is
> not received, before the timeout elapses. I would expect that if one
> was to observe the amount of time for the CF to complete it should
> be approximately equal to the overall timeout.
>
>>> Ignoring connect for now, this means that
>>> the timer starts from the initial DNS lookup/TCP connection
>>
>> Confused. You are not ignoring the connect if you take into account
>> DNS and TCP connection establishment.
>
> Sorry, I meant ignoring an explicit `connectTimeout` method for now,
> then the amount of time spent setting up the connection should be
> considered part of the overall time spent for the timer. More on this
> below.
>
>>> and ends
>>> when the complete request has been sent and the complete response
>>> has been received. This seems like the most useful overall timeout
>>> semantic.
>>
>> Indeed.
>> Note that it's indeed request *and* response completion, including
>> data for both.
>
> Right. I just wanted to confirm this explicitly ( since the current timeout
> implementation stops the timer after the response headers are
> received ). Changing this will take time.
>
>> That means if the request takes longer than the response, it'll
>> possible trigger _after_ the complete response has arrived to the
>> client.
>
> True, at which point there may not be a mechanism available to
> report the error, beyond cancelling the request body’s publisher,
> which may be enough in itself to indicate that the whole request
> body was not sent.
>
>>> There is a question about whether or not the response body is part
>>> of the timeout. It currently is not since the body handler API has the
>>> ability to either stream the response body or return it as a higher-level
>>> Java Object, e.g String, etc.
>>
>> IMHO it should be.
>
> I think so too. I’m concerned though that attempting to change this
> now could destabilise things. I’m working on a prototype, but we may
> need a backup evolutionary plan that allows this to be adjusted post
> JDK 11.
>
>> Response content is where things may go really
>> slow, as the HTTP response headers are typically delivered in just one
>> TCP frame (for both HTTP1 and HTTP2).
>
> Right.
>
>>> I agree with adding `connectTimeout` to HttpClient.
>>
>> Great.
>
> Ok. We have agreement here.
>
>>> The low-level details
>>> of what constitutes a new underlying connection is somewhat known. I
>>> agree that it should take into account such things as DNS lookup, TCP
>>> connect, and SSL handshake.
>>
>> As I said, I'm not sure about the latter, but either works for me.
>
> Ok.
>
>>> Summary of modes:
>>>
>>> 1) HttpRequest `timeout` only - times both connect ( if any ) and
>>> until both the request and the response are fully send and received,
>>> respectively.
>>
>> Yes, including - for both - their respective content.
>
> Yes.
>
>>> 2) HttpClient `connectTimeout` only - times only the connect phase,
>>> if any. Timer stops after connection has been established.
>>
>> Yes.
>>
>>> 3) HttpClient `connectTimeout` and HttpRequest `timeout` - start
>>> connect phase timer at DNS lookup, stop at TCP connect finished
>>> or SSL handshake complete. Start request specific timer shortly
>>> after connect timer stops, i.e. just before the HTTP request is sent.
>>> Stop request specific timer when both the request and the response
>>> are fully send and received, respectively.
>>
>> Seems to me that this contradicts what you said before (total timeout
>> includes the connectTimeout).
>
> What I meant was: Total timeout includes the time to connect if an explicit
> connect timeout is not specified. If an explicit connect timeout is specified
> then the connect part is timed by it, and the total timeout only starts after
> the connection is setup. Anyway, what you outlined below may be more
> straight forward.
>
>> Given exchange = request+response, then:
>> 1. send request
>> 2. start "total" timer
>> 3a. need to open connection, start "connectTimeout" timer and try to
>> open connection; if connectTimeout elapses, "total" timeout is
>> cancelled, and exchange is failed; if connection is opened, cancel
>> connectTimeout.
>> 3b. connection available, send request.
>> 3c. connections exist, but none is available to send (all busy with
>> other exchanges and cannot open a new connection due to capping of
>> number of connections to same origin), request is queued.
>> 4. "total" timeout elapses, exchange can be: queued, sending or
>> receiving; in all cases fail the exchange.
>> 5. exchange completes in time, cancel "total" timeout.
>
> Implementing the connect timeout part of this seems reasonably straight
> forward, and I have a few prototypes that I hacked on over the weekend.
> They are stable and well tested ( some work remains on intermediate
> responses/requests, like authentication and redirection ).
>
> What this discussion has uncovered is the, justifiable, desire to time the
> complete response body. The current timeout mechanism does not do
> this, and has never done this ( the timer stops once the response headers
> are received ). I want to prototype changing this to cover the complete
> response body and see how it goes.
>
> -Chris.
>
