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
