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
8289291: HttpServer sets incorrect value for "max" parameter in Keep-Alive header value #9326
Conversation
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
Webrevs
|
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.
Code change looks fine. Could we add a test (even to an existing keepalive one) to verify that "max" is no longer sent?
…sent in Keep-Alive response header
Done. I've updated the PR to include a new test which verifies the absence of the |
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.
Looks fine. Thanks!
@jaikiran 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 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Just wondering about using a simple Socket as the client (which is fine). I'd expect it should be possible to see the value of the header with the new HttpClient (unless we filter it?). |
The Http(s)Server will set the |
Thank you Michael and Daniel for the reviews. |
/integrate |
Going to push as commit dddd4e7.
Your commit was automatically rebased without conflicts. |
Can I please get a review for this change which addresses https://bugs.openjdk.org/browse/JDK-8289291?
As noted in that issue, right now, the Http(s)Server sets an incorrect value for the
max
parameter of theKeep-Alive
header. Themax
value is supposed to be the number of subsequent requests that the server is willing to serve over that specific connection. The current value it sets is instead the number of idle connections that are configured for the server.The commit in this PR removes that
max
parameter altogether, since it isn't mandated by the spec, nor does the Http(s)Server have any specific construct to come up with a right value. Furthermore, on the client side the HttpURLConnection based client doesn't mandate the presence of this parameter in theKeep-Alive
header. So this change won't cause any regressions in that area.tier1, tier2 and tier3 testing passed without any related issues after this change.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9326/head:pull/9326
$ git checkout pull/9326
Update a local copy of the PR:
$ git checkout pull/9326
$ git pull https://git.openjdk.org/jdk pull/9326/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9326
View PR using the GUI difftool:
$ git pr show -t 9326
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9326.diff