RFR: 8276798: HttpURLConnection sends invalid HTTP request

[Daniel Fuchs]

On Mon, 6 Jun 2022 09:43:50 GMT, Jaikiran Pai <jpai at 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