RFR [12] 8218546 : Unable to connect to https://google.com using java.net.HttpClient

[Chris Hegarty]

Thanks for the comments Daniel. I made some simplifications to the test
and it now covers the small missing case and is more readable. 

Updated webrev:
  http://cr.openjdk.java.net/~chegar/8218546/webrev.01/

-Chris.

> On 7 Feb 2019, at 13:22, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Chris,
> 
> The code changes look good to me. They are very limited
> and look safe and reasonable.
> The test looks good too - though it's becoming a bit more
> difficult to read, especially with the mysterious i > 0.
> 
> I'd suggest adding a comment (or some asserts)
> in the else branch of the if (lines  201 and 309)
> to make it a bit more obvious that the branch is
> entered only when:
> 
> we are checking the "host" header, *AND*
> res.version is HTTP/2, *AND*
> no custom header was specified in the request, so
> we're just checking the expected default value that
> the client should have sent.
> 
> For instance, adding these 7 lines could fit the bill:
> 
> assert name.equalsIgnoreCase("host");
> assert useDefault;
> assert resp.version() == HTTP_2;
> // In that case, and excluding the response to the HTTP/1.1 upgrade
> // which would have been sent through HTTP/1.1, the server's
> // handler should not have found any "host" header in the request
> // headers, as HTTP/2 use :authority instead.
> 
> BTW: doesn't that leave a tiny hole for the upgrade case? Or is
> it simply too difficult to figure out whether the connection
> actually started with HTTP/1.1 and was upgraded?
> 
> 
> best regards,
> 
> -- daniel
> 
> On 07/02/2019 12:48, Chris Hegarty wrote:
>> This is a code review request for a late fix for JDK 12, to address a
>> potentially serious regression.
>> Post 8213189 [1] the HTTP Client now adds both the `:authority:`
>> pseudo-header and the `Host` header to outbound HTTP/2 requests. The
>> HTTP/2 RFC does not seem to rule out this behavior, but it does
>> recommend that the pseudo-header be used *instead* of the `Host` header.
>> I'm not quite sure why the Goog server rejects such a request ( with
>> both headers ). Other HTTP/2 servers that I have checked do not. While
>> the behaviour of the Goog server is questionable, the fact remains that
>> the HTTP Client cannot successfully interact with it over HTTP/2. The
>> behaviour of the client, post 8213189, has deviated away from what is
>> recommended by the RFC. On balance, it seems safest to just revert the
>> small part of the change for 8213189 that caused this issue. The small
>> part is not even all that relevant to the crux of the fix for 8213189.
>> Bug:
>>   https://bugs.openjdk.java.net/browse/JDK-8218546
>> Webrev:
>>   https://cr.openjdk.java.net/~chegar/8218546/webrev.00/
>> -Chris.
>> [1] https://bugs.openjdk.java.net/browse/JDK-8213189
>