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

[Daniel Fuchs]

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
>