Skip to content

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Aug 2, 2024

See the issue for more discussion and rationale. This is the actual code change, should we decide to go forward.

This is a git revert of b37df14.

Additional testing:

  • Linux AArch64 server fastdebug, jdk_net
  • Linux AArch64 server fastdebug, all

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • JDK-8337684 needs maintainer approval

Issue

  • JDK-8337684: [17/11u] Revert JDK-8163921 backport (Bug - P4 - Requested)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2775/head:pull/2775
$ git checkout pull/2775

Update a local copy of the PR:
$ git checkout pull/2775
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2775/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2775

View PR using the GUI difftool:
$ git pr show -t 2775

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2775.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 2, 2024

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Aug 2, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 2, 2024
@mlbridge
Copy link

mlbridge bot commented Aug 2, 2024

Webrevs

@openjdk
Copy link

openjdk bot commented Aug 16, 2024

⚠️ @shipilev This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

@shipilev
Copy link
Member Author

/approval request Cleanly reverts JDK-8163921. See the comments in the issue for more discussion and rationale. jdk_net tests pass. This reverts us to a known good point before JDK-8163921, so the risk should be on lower side.

@openjdk
Copy link

openjdk bot commented Aug 19, 2024

@shipilev
8337684: The approval request has been created successfully.

@openjdk openjdk bot added the approval Requires approval; will be removed when approval is received label Aug 19, 2024
@jerboaa
Copy link
Contributor

jerboaa commented Aug 20, 2024

I'm OK with this change. Do you plan on doing a more targeted fix for https://bugs.openjdk.org/browse/JDK-8163921 (e.g. by fixing the syntax error in the default accept header only)?

@shipilev
Copy link
Member Author

I'm OK with this change. Do you plan on doing a more targeted fix for https://bugs.openjdk.org/browse/JDK-8163921 (e.g. by fixing the syntax error in the default accept header only)?

Not at this time. The far-reaching unintended consequences of the original fix tells me this is a fiddly piece of code, so we are a bit averse trying to do more things than necessary here. I think we just let JDK 11 and 17 be as they are, even though it leaves HTTP client de-jure non-compliant to RFC. Let applications first encounter any default policy change in JDK 21.

@jerboaa
Copy link
Contributor

jerboaa commented Aug 21, 2024

@GoeLin What's your take on this? If we revert, we should probably get it in soon.

@shipilev
Copy link
Member Author

shipilev commented Sep 2, 2024

@GoeLin What's your take on this? If we revert, we should probably get it in soon.

Also interested in this answer :) We missed the normal cutoff for October releases, but I can resubmit this PR as 17u-critical.

@GoeLin
Copy link
Member

GoeLin commented Sep 2, 2024

Has anybody else seen this, besides Amazon in its internal systems?
I'm not so sure this is of relevance for everybody.
I think it's quite obvious that, if someone programmed against the wrong behaviour, this fix causes an issue. But this holds for this kind of fixes in general, so why is this special enough to justify a backout?

@GoeLin
Copy link
Member

GoeLin commented Sep 3, 2024

Hi @shipilev
What do you think about this? #2843

It implements what Daniel proposes in his comment in 8163921

@shipilev
Copy link
Member Author

Closing in favor of #2843

@shipilev shipilev closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval Requires approval; will be removed when approval is received rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

4 participants