Thanks for the updates, Chris. This looks fine to me (I'm not a reviewer).
-Jaikiran
On 07/08/18 3:22 PM, 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.
>
