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.fu...@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 >
