The updated webrev looks fine to me, Chris.
Thanks,
Michael
On 07/08/2018, 10:52, Chris Hegarty wrote:
On 4 Aug 2018, at 14:08, Jaikiran Pai<jai.forums2...@gmail.com> wrote:
...
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}.
Agreed. This wording avoids the previous awkwardness.
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?
Accidental, left over from a previous hacking session. Removed.
Updated webrev:
http://cr.openjdk.java.net/~chegar/8208391/webrev.01/
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
I am aware of this.
. Does the HttpClient
honour that system property in the exceptions it throws?
The HTPT Client does not have any special handing for this property.
It may not be necessary, at least not for low-level NIO exceptions, once
the exception, or its cause, is preserved.
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.
Separately, I will look into the possibility of where such an extension
can be used.
-Chris.
