-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8358942: HttpClient adds Content-Length: 0 for a GET request with a BodyPublishers.noBody() #27727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into |
|
@djelinski This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 118 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@djelinski The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| HttpResponse<String> resp = hc.send(req, HttpResponse.BodyHandlers.ofString(UTF_8)); | ||
| assertEquals(resp.statusCode(), 200, resp.body()); | ||
| assertEquals(resp.version(), version); | ||
| assertEquals(resp.body(), "Request completed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, you may consider verifying the response body in other test methods too, since, in particular, OptionalContentLengthHandler responds with 200 in two different scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not possible in most cases: in HTTP/2 and HTTP/3 we don't send the content-length:0 header, we only send content-length when it's known up front and non-zero.
In cases where it is possible it's usually not necessary - most of the time we don't really care if the content length was sent or not, as long as the server is able to figure it out.
I'll add a check for the HTTP/1 + PUT/POST case, since that's special-cased by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken in case of H2/H3 you could verify that:
assertTrue(List.of("Request completed, no content length", "Request completed").contains(resp.body()), "Unexpected response: " + resp.body());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up removing the OptionalContentLengthHandler. It didn't feel right anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OptionalContentLengthHandler gone feels better. This also implies that the check is stricter: we either expect the Content-Length, or not.
Why is this an HTTP/1.1-only problem?HTTP/2 and HTTP/3 implementations of jdk/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java Lines 906 to 908 in 4728f74
jdk/src/java.net.http/share/classes/jdk/internal/net/http/Http3ExchangeImpl.java Lines 388 to 390 in 4728f74
They do this, unlike the
Credits: Thanks to @dfuch and @djelinski for patiently answering my questions. |
| HttpResponse<String> resp = hc.send(req, HttpResponse.BodyHandlers.ofString(UTF_8)); | ||
| assertEquals(resp.statusCode(), 200, resp.body()); | ||
| assertEquals(resp.version(), version); | ||
| assertEquals(resp.body(), "Request completed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OptionalContentLengthHandler gone feels better. This also implies that the check is stricter: we either expect the Content-Length, or not.
|
@djelinski you should probably Finalize the CSR JDK-8369533 now. |
|
Thanks for the reviews! /integrate |
|
Going to push as commit ead35a7.
Your commit was automatically rebased without conflicts. |
|
@djelinski Pushed as commit ead35a7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Do not send the Content-Length header on HTTP/1.1 requests when the content length is known to be zero and the method does not expect content or is unknown.
This brings the HTTP/1.1 implementation in line with the recommendations from RFC 9110.
The existing ContentLengthHeaderTest was extended to cover the modified scenarios.
Tier1-3 tests continue to pass.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27727/head:pull/27727$ git checkout pull/27727Update a local copy of the PR:
$ git checkout pull/27727$ git pull https://git.openjdk.org/jdk.git pull/27727/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27727View PR using the GUI difftool:
$ git pr show -t 27727Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27727.diff
Using Webrev
Link to Webrev Comment