Hi,
On Tue, Jul 31, 2018 at 9:58 PM Chris Hegarty <chris.hega...@oracle.com> wrote:
>
> It is unfortunate that this has come up so late the JDK 11 lifecycle,
> but since this has API impact, and is the right thing to do, it is
> certainly worth addressing now ( rather than waiting to do something in
> a future release).
>
> TL;DR provide API support for setting connection and response specific
> timeouts, while continuing to support a single overall request timeout.
>
> The following issue has been file to track this:
> https://bugs.openjdk.java.net/browse/JDK-8208391
>
> Propose
> -------
>
> In the `HttpRequest.Builder` class remove the single `timeout` method
> and replace it with a pair of `connectTimeout` and `responseTimeout`.
> The `connectTimeout` method can be used to set a timeout duration that
> is specific to the connection phase. The `responseTimeout` method can be
> used to set a timeout for the actual of the HTTP response. For example,
>
> HttpRequest request = HttpRequest.newBuilder()
> .uri(URI.create("https://foo.com/";))
> .connectTimeout(Duration.ofSeconds(20))
> .responseTimeout(Duration.ofMinutes(1))
> .header("Content-Type", "application/json")
> .POST(BodyPublishers.ofFile(Paths.get("file.json")))
> .build();
>
> We consider the ability to be able to set a single timeout for the
> overall response to be important, so the `responseTimeout` method can be
> used to set an overall response timeout, in the absence of a more
> specific `connectTimeout`. This means that the newly named
> `responseTimeout` is semantically equivalent to the old `timeout` when
> used without the new `connectTimeout`.
I don't think this is precisely specified.
AFAIU, connectTimeout is the time to perform DNS lookup plus the time
to establish the TCP connection (SYN+SYN/ACK+ACK), so you may want to
clarify that.
But responseTimeout also is not precisely defined, and assumes that a
response starts after a request, but this is not always the case.
Imagine the case of uploading a huge file to the server on a slow network.
The server may reply with a 500 immediately so the response side is
finished almost immediately, but the request content upload may
continue for minutes after the response is arrived, depending on the
server implementation.
The good case, where the huge upload continues normally and completes
before the response is sent by the server, is difficult to quantify
beforehand and therefore to configure on the request object (as it
depends on the network).
Point being that "responseTimeout" does not convey the fact that in
certain cases 99% of the time can be spent in the request, not in the
response.
> /**
> * Sets the response timeout duration for this request.
> *
> * <p> If the response headers are not received within the specified
> * duration, then an {@link HttpTimeoutException} is thrown from {@link
> * HttpClient#send(java.net.http.HttpRequest,
> * java.net.http.HttpResponse.BodyHandler) HttpClient::send}, or
> * {@link HttpClient#sendAsync(java.net.http.HttpRequest,
> * java.net.http.HttpResponse.BodyHandler) HttpClient::sendAsync}
> * completes exceptionally with an {@code HttpTimeoutException}.
> *
> * <p> If a {@linkplain #connectTimeout(Duration) connect timeout
> duration}
> * has also been specified, then this timeout duration starts after the
> * connection has been established, and the connection setup phase is
> * timed separately by the connect timeout duration. If no connect
> * timeout duration has been specified, then this timeout duration
> * covers both connection setup and receiving of the response.
> *
> * <p> The effect of not setting a timeout duration is the same as
> * setting an infinite duration, ie. block forever, or the Operating
> * System default.
> *
> * @param duration the duration to allow the response headers to be
> * received
> * @return this builder
> * @throws IllegalArgumentException if the duration is non-positive
> */
> public abstract Builder responseTimeout(Duration duration);
Again, this is not clear.
If the implementation already has setup a connection (because it pools
them, or in the HTTP/2 case), it is not clear when this
responseTimeout starts.
Does it start _after_ the whole request has been sent?
Does it start as soon as the request is sent?
If the goal is to limit the total request+response time (and by
experience it is what generally people want), then I think the name
"timeout" as it was before is a better choice and needs to be defined
as request+response timeout.
It should also be clarified what happens in case of conversations such
as redirects or authentication challenges: it is the whole
conversation subject to the timeout, or single request+response pairs?
> /**
> * Sets the connect timeout duration for this request.
> *
> * <p> In the case where a new connection needs to be established, if
> * the connection cannot be established within the given {@code
> * duration}, then a {@link HttpConnectTimeoutException} is thrown
> * from {@link HttpClient#send(java.net.http.HttpRequest,
> * java.net.http.HttpResponse.BodyHandler) HttpClient::send}, or
> * {@link HttpClient#sendAsync(java.net.http.HttpRequest,
> * java.net.http.HttpResponse.BodyHandler) HttpClient::sendAsync}
> * completes exceptionally with a {@code HttpConnectTimeoutException}.
> * If a new connection does not need to be established, for example
> * if a connection can be reused from a previous request, then this
> * timeout duration has no effect.
> *
> * <p> The effect of not setting a connect timeout duration is defined
> * in the method {@link #responseTimeout(Duration) responseTimeout}.
> *
> * @param duration the duration to allow the underlying connection to be
> * established
> * @return this builder
> * @throws IllegalArgumentException if the duration is non-positive
> */
> public abstract Builder connectTimeout(Duration duration);
Why this is on the request builder and not in the HttpClient builder?
The connectTimeout is not a request property, it is a HttpClient
property as it is HttpClient that opens (to pool them) connections,
and this is even more true for HTTP/2 where apart the first request
possibly no other connections are opened towards the same origin.
Imagine in the future you want to add a functionality that pre-opens
connections to specified origins to reduce the latency when sending
requests, similar to ThreadPoolExecutor.prestartAllCoreThreads().
I would argue that you would put this method in HttpClient and so it
is HttpClient that needs to be configured with the connectTimeout.
I propose to keep "timeout" as a total request+response timeout (and
possibly conversation timeout, your call) on the request builder, and
to move "connectTimeout" to the HttpClient builder, specifying that it
means DNS lookup plus TCP connection establishment.
In case a request needs to open a connection, "timeout" will still be
the total time from when the request is sent to when both request and
response are finished, including the connectTimeout.
By having a typically shorter connectTimeout (say 5 seconds) and a
longer total timeout (say 2 minutes), a request will fail in 5 seconds
if HttpClient fails to open a connection to the origin server, and
fail in 2 minutes if it takes too much.
Finally, I would consider TLS handshake time as part of the request
data: the cost will be paid by the first request and capped by "total"
timeout (not connectTimeout).
With TLS 1.3 optimizations, it may be just few TLS bytes before the
encrypted request bytes.
Thanks!
--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
