Hi Chris,
src/java.net.http/share/classes/java/net/http/HttpClient.java
+ * <p> In the case where a new connection needs to be
established, if
+ * the connection cannot be established within the given {@code
+ * duration}, then an {@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 an {@code
HttpConnectTimeoutException}.
Do you think this can be reworded a bit? Although I understand what's
being said here, the wording doesn't seem right. Maybe something like:
* <p> In the case where a new connection needs to be
established, if
* the connection cannot be established within the given {@code
* duration}, then {@link HttpClient#send(java.net.http.HttpRequest,
* java.net.http.HttpResponse.BodyHandler) HttpClient::send}
throws a
* {@link HttpConnectTimeoutException}, or
* {@link HttpClient#sendAsync(java.net.http.HttpRequest,
* java.net.http.HttpResponse.BodyHandler) HttpClient::sendAsync}
* completes exceptionally with an {@code
HttpConnectTimeoutException}.
src/java.net.http/share/classes/jdk/internal/net/http/HttpRequestImpl.java
- this.timeout = null;
+ this.timeout = Duration.ofSeconds(30);
Is this an intentional change of default value for the timeout? Is that
something that needs to be documented?
One other thing - maybe not directly related to this single patch, but
as you are aware, recently as part of [1], a new system property (and a
security property) was introduced to optionally include host info in the
exception messages thrown for socket exceptions. Does the HttpClient
honour that system property in the exceptions it throws? I don't see it
being used in this patch for the timeout exceptions. I haven't checked
the code, outside of this patch, to see if it's dealt with in other
parts of this API.
[1] https://bugs.openjdk.java.net/browse/JDK-8204233
-Jaikiran
On 04/08/18 5:15 PM, Chris Hegarty wrote:
> Hi, This is a code review to add connection specific timeout support
> to the new HTTP Client, as discussed recently over on another thread
> [1]. The connection timeout duration is proposed to be added at the
> level of the client, and if specified applies to all requests sent by
> that client. The connect timeout mechanism is orthogonal to the
> HttpRequest `timeout`, but can be used to complement it. For example,
> HttpClient client = HttpClient.newBuilder()
> .connectTimeout(Duration.ofSeconds(20)) .build(); HttpRequest request
> = HttpRequest.newBuilder() .uri(URI.create("https://foo.com/";))
> .timeout(Duration.ofMinutes(2)) .header("Content-Type",
> "application/json")
> .POST(BodyPublishers.ofFile(Paths.get("file.json"))) .build();
> client.sendAsync(request, BodyHandlers.ofString()) sends the request
> with a 20 second connect timeout and a response timeout of 2 minutes.
> The connect timeout, as implemented in the webrev below, covers the
> DNS lookup, the TCP SYN handshake, and the TLS handshake. The proposed
> spec wording deliberately avoids adding such low-level details.
> Outline of the specification changes: To HttpClient.Builder add: /** *
> Sets the connect timeout duration for this client. * * <p> In the case
> where a new connection needs to be established, if * the connection
> cannot be established within the given {@code * duration}, then an
> {@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 an {@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. * * @param duration the duration to
> allow the underlying connection to be * established * @return this
> builder * @throws IllegalArgumentException if the duration is
> non-positive */ public Builder connectTimeout(Duration duration); To
> HttpClient add an accessor: /** * Returns an {@code Optional}
> containing the <i>connect timeout duration</i> * for this client. If
> the {@linkplain Builder#connectTimeout(Duration) * connect timeout
> duration} was not set in the client's builder, then the * {@code
> Optional} is empty. * * @return an {@code Optional} containing this
> client's connect timeout * duration */ public abstract
> Optional<Duration> connectTimeout(); Add new Exception type: /** *
> Thrown when a connection, over which an {@code HttpRequest} is
> intended to be * sent, is not successfully established within a
> specified time period. */ public class HttpConnectTimeoutException
> extends HttpTimeoutException { ... } Webrev:
> http://cr.openjdk.java.net/~chegar/8208391/webrev.00/ -Chris [1]
> http://mail.openjdk.java.net/pipermail/net-dev/2018-August/011683.html
