On Mon, 6 Jun 2022 09:43:50 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this change which proposes to fix > https://bugs.openjdk.java.net/browse/JDK-8276798? > > `sun.net.www.protocol.http.HttpURLConnection` has a (private) `writeRequests` > method. This method is responsible for creating the standard HTTP request > headers (include the request line) and then writing it out to the > `OutputStream` which communicates with the HTTP server. While writing out > these request headers, if there's an IOException, then `HttpURLConnection` > marks a `failedOnce` flag to `true` and attempts to write these again afresh > (after creating new connection to the server). This re-attempt is done just > once. > > As noted in the linked JBS issue, specifically this comment > https://bugs.openjdk.java.net/browse/JDK-8276798?focusedCommentId=14500074&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14500074, > there's a specific case where creating and writing out these request headers > ends up skipping the request line, which causes the server to respond back > with a "Bad Request" response code. > > The commit in this PR removes the use of `failedOnce` flag that was being > used to decide whether or not to write the request line. The existing code > doesn't have any specific comments on why this check was there in first > place, nor does the commit history show anything about this check. However, > reading through that code, my guess is that, it was there to avoid writing > the request line twice when the same `requests` object gets reused during the > re-attempt. I think a better check would be the see if the `requests` already > has the request line and if not add it afresh. > While in this code, I also removed the check where the `failedOnce` flag was > being used to check if the `Connection: Keep-Alive`/`Proxy-Connection: > Keep-alive` header needs to be set. This part of the code already has a call > to `setIfNotSet`, so I don't think we need the `failedOnce` check here. > > tier1, tier2 and tier3 tests have passed without issues. However, given the > nature of this code, I'm not too confident that we have tests which can catch > this issue (and adding one isn't easy), so I would like inputs on whether > this change is good enough or whether it has the potential to cause any > non-obvious regressions. This looks reasonable to me but I would like to get a second opinion. @Michael-Mc-Mahon ? ------------- Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9038
