Skip to content
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

8335181: Incorrect handling of HTTP/2 GOAWAY frames in HttpClient #1020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryanflegel
Copy link

@ryanflegel ryanflegel commented Oct 3, 2024

I am backporting this fix because I am being affected by JDK-8335181. I had to downgrade to HTTP/1.1 in order to use HttpClient.

I needed to resolve two minor conflicts in MultiExchange.java due to JDK-8191494 being applied slightly differently in different projects. In both cases, I kept the change closer to upstream.

The conflicts were:

  • Explicit vs implicit null field initialization
  • Different debug log message

Tests

All tests in the GitHub actions have passed.

I have run the jdk/java/net tests locally and there were several failures, including 4 within java/net/httpclient. However, I have confirmed that all the failed tests also fail before applying the patch, so they seem to be unrelated.

Here is a full list of the failures:

  • java/net/DatagramSocket/B6411513.java: java.net.DatagramSocket.receive: packet isn't received
  • java/net/DatagramSocket/SendReceiveMaxSize.java: This test verifies that on macOS, the send buffer size is configured by default so that none of our implementations of the UDP protocol will fail with a "packet too large" exception when trying to send a packet of the maximum possible size allowed by the protocol.
  • java/net/httpclient/ConnectTimeoutNoProxyAsync.java: Tests for connection related timeouts
  • java/net/httpclient/ConnectTimeoutNoProxySync.java: Tests for connection related timeouts
  • java/net/httpclient/ConnectTimeoutWithProxyAsync.java: Tests for connection related timeouts
  • java/net/httpclient/ConnectTimeoutWithProxySync.java: Tests for connection related timeouts

UPDATE: I've run the rest of the tier2 tests and there were two additional JDK failures that appear to be unrelated:

  • com/sun/jndi/dns/ConfigTests/Timeout.java: Tests that we can set the initial UDP timeout interval and the number of retries.
  • com/sun/jndi/ldap/LdapPoolTimeoutTest.java: Multi-threaded client timeout tests for ldap pool

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-8335181 needs maintainer approval

Issue

  • JDK-8335181: Incorrect handling of HTTP/2 GOAWAY frames in HttpClient (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1020

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 3, 2024

👋 Welcome back ryanflegel! 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 Oct 3, 2024

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

@openjdk openjdk bot changed the title Backport 720b44648bcff997278af92746f942b2425298a5 8335181: Incorrect handling of HTTP/2 GOAWAY frames in HttpClient Oct 3, 2024
@openjdk
Copy link

openjdk bot commented Oct 3, 2024

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Oct 3, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 3, 2024

Webrevs

@jaikiran
Copy link
Member

jaikiran commented Oct 4, 2024

Hello Ryan,

I was unable to run local tests due to this error:

* For target jdk__optimize_image_exec:
Error: Unable to initialize main class build.tools.jigsaw.AddPackagesAttribute
Caused by: java.lang.ClassFormatError: StackMapTable format error: access beyond the end of attribute

If you are on macosx and using Xcode 16 then I suspect you are running into the issue noted in https://mail.openjdk.org/pipermail/jdk-dev/2024-September/009400.html

@ryanflegel
Copy link
Author

Thanks @jaikiran. That's almost certainly what the issue is. I'll downgrade to XCode 15.4 and give the tier2 tests another try.

@ryanflegel
Copy link
Author

I was able to get tests running after downgrading XCode and have updated the PR description with the results.

@ryanflegel
Copy link
Author

/label net

@openjdk
Copy link

openjdk bot commented Oct 9, 2024

@ryanflegel
The label net is not a valid label.
These labels are valid:

@ryanflegel
Copy link
Author

Testing is complete for this PR and it is ready for review. Is there someone that can take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants