RFR [11] 8208391: Need to differentiate response and connect timeouts in HTTP Client API

[Michael McMahon]

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.forums2013 at 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.
>