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

8170305: URLConnection doesn't handle HTTP/1.1 1xx (informational) messages #10229

Closed
wants to merge 6 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Sep 9, 2022

Can I please get a review of this change which proposes to fix https://bugs.openjdk.org/browse/JDK-8170305?

The commit in this PR changes the internal implementation of HttpURLConnection to ignore interim informational 1xx responses from server and continue to wait for the final response. This is a similar fix to what we did in the new HttpClient API in #10169.

A new test has been added to use the HttpURLConnection to reproduce the issue and verify the fix.

tier1, tier2 and tier3 testing is in progress with this change.


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

Issue

  • JDK-8170305: URLConnection doesn't handle HTTP/1.1 1xx (informational) messages

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10229/head:pull/10229
$ git checkout pull/10229

Update a local copy of the PR:
$ git checkout pull/10229
$ git pull https://git.openjdk.org/jdk pull/10229/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10229

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10229.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 9, 2022

👋 Welcome back jpai! 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 openjdk bot added the rfr Pull request is ready for review label Sep 9, 2022
@openjdk
Copy link

openjdk bot commented Sep 9, 2022

@jaikiran The following label will be automatically applied to this pull request:

  • net

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.

@openjdk openjdk bot added the net net-dev@openjdk.org label Sep 9, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 9, 2022

Webrevs

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM globally. We're not supposed to receive 101 if we didn't ask for an upgrade (and we never should), but what happens if we still receive 101?
It could be beneficial to add that to your test as well.

…ttpURLConnection doesn't ask for a connection upgrade
@jaikiran
Copy link
Member Author

Hello Daniel,
I've now updated the PR to include a test for 101 as well as updated the source to handle the 101 differently than other interim 1xx responses. Since HttpURLConnection doesn't support connection upgrade, the receipt of a 101 response from a server is considered an error and with the change in this PR, the request will now fail and we internally close the connection as well.

I've triggered tier1, tier2 and tier3 to make sure this doesn't cause any issues.

@Michael-Mc-Mahon
Copy link
Member

I just tested the latest patch and the new test hangs for me. I can send you the stack trace to take a look.

@jaikiran
Copy link
Member Author

Hello Michael, I could reproduce the hang locally after a few attempts. This was a bug in the test server which I've now fixed in the updated version of the PR.

What was happening was that the client upon seeing an unexpected 101 would close the connection and throw a (client side) IOException. The HttpURLConnection however has certain code which retries a (GET) request on IOException. The test server upon seeing a connection closure from the client would stop accept()ing any more connections but would still have an ESTABLISHED server socket listening on that host/port. So with no one "accepting" the connection, the client side would time out waiting for the server to respond. This has now been fixed in the test specific server and I've run this test a few times to make sure this now passes. I'll also run some CI runs with a --test-repeat just to be sure it doesn't have any other issues.

@jaikiran
Copy link
Member Author

tier1, tier2, tier3 testing as well as testing of test/jdk/java/net/HttpURLConnection/ with a test-repeat of 50 completed without any issues, with the latest state of this PR.

System.out.println("Issuing request to " + requestURI);
final HttpURLConnection urlConnection = (HttpURLConnection) requestURI.toURL().openConnection();
// we expect the request to fail because the server unexpectedly sends a 101 response
Assert.assertThrows(IOException.class, () -> urlConnection.getResponseCode());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we explicitly wait for ProtocolException here, since it's what we're throwing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought about that, but since the API javadoc doesn't specifically talk about this sub-type of IOException, I decided it's perhaps better not to start expecting it. However, if it's OK for the test to rely on this (internal) knowledge then I'll go ahead and change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - this is not a conformance test, and ProtocolException is within the bounds. It's OK for a non-regression test to expect the specific subtype that the code change is throwing. My concern here is that the test could be passing if some other kind of errors occurred - like e.g. connect exception. Then we wouldn't be testing what we think we're testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I've now updated the test to expect ProtocolException. The test continues to pass with this change.

@openjdk
Copy link

openjdk bot commented Sep 12, 2022

@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:

8170305: URLConnection doesn't handle HTTP/1.1 1xx (informational) messages

Reviewed-by: dfuchs, michaelm

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 17 new commits pushed to the master branch:

  • 9ef6c09: 8287908: Use non-cloning reflection methods where acceptable
  • 0c61bf1: 8293282: LoadLibraryUnloadTest.java fails with "Too few cleared WeakReferences"
  • 91c9091: 8293343: sun/management/jmxremote/bootstrap/RmiSslNoKeyStoreTest.java failed with "Agent communication error: java.io.EOFException"
  • 4c77bd3: 6529151: NullPointerException in swing.plaf.synth.SynthLookAndFeel$Handler
  • d5aae01: 8293544: G1: Add comment in G1BarrierSetC1::pre_barrier
  • 37df5f5: 8291599: Assertion in PhaseIdealLoop::skeleton_predicate_has_opaque after JDK-8289127
  • 699c429: 8292866: Java_sun_awt_shell_Win32ShellFolder2_getLinkLocation check MultiByteToWideChar return value for failures
  • 68da02c: 8292240: CarrierThread.blocking not reset when spare not activated
  • 005b49b: 8293044: C1: Missing access check on non-accessible class
  • 91d00b3: 8288473: Remove unused frame::set_pc_preserve_deopt methods
  • ... and 7 more: https://git.openjdk.org/jdk/compare/9d6b0285f53599816c30393b87d16772ef6314b7...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2022
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jaikiran
Copy link
Member Author

Thank you Daniel and Michael for the reviews.

@jaikiran
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 13, 2022

Going to push as commit 8bd79d3.
Since your change was applied there have been 27 commits pushed to the master branch:

  • 9cd3e35: 4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click
  • ec2629c: 8275275: AArch64: Fix performance regression after auto-vectorization on NEON
  • cbee0bc: 8292587: AArch64: Support SVE fabd instruction
  • 68645eb: 8293566: RISC-V: Clean up push and pop registers
  • 526eb54: 8293669: SA: Remove unnecssary "InstanceStackChunkKlass: InstanceStackChunkKlass" output when scanning heap
  • 41ce658: 8292225: Rename ArchiveBuilder APIs related to source and buffered addresses
  • 155b10a: 8293329: x86: Improve handling of constants in AES/GHASH stubs
  • d3f7e3b: 8293339: vm/jvmti/StopThread/stop001/stop00103 crashes with SIGSEGV in Continuation::is_continuation_mounted
  • 524af94: 8283627: Outdated comment in MachineDescriptionTwosComplement.isLP64
  • cea409c: 8292738: JInternalFrame backgroundShadowBorder & foregroundShadowBorder line is longer in Mac Look and Feel
  • ... and 17 more: https://git.openjdk.org/jdk/compare/9d6b0285f53599816c30393b87d16772ef6314b7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 13, 2022
@openjdk openjdk bot closed this Sep 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 13, 2022
@openjdk
Copy link

openjdk bot commented Sep 13, 2022

@jaikiran Pushed as commit 8bd79d3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jaikiran jaikiran deleted the 8170305 branch September 13, 2022 05:09
@reschke
Copy link

reschke commented Sep 13, 2022

Thanks - I'll test once the next build is out.

Is this something that would be a candidate for backporting?

@jaikiran
Copy link
Member Author

Hello Julian,

Is this something that would be a candidate for backporting?

This change is relatively straightforward and backporting should be OK. OpenJDK backports however are decided/managed by different team(s) and discussed in https://mail.openjdk.org/mailman/listinfo/jdk-updates-dev mailing list. The OpenJDK guide has more details on the backporting process https://openjdk.org/guide/#backporting

@reschke
Copy link

reschke commented Sep 17, 2022

Tested over at https://github.com/greenbytes/java-http-1xx-tests and found to be working; thanks!

OpenJDK backports however are decided/managed by different team(s) and discussed in https://mail.openjdk.org/mailman/listinfo/jdk-updates-dev mailing list.

So is the proposal that I should follow up over there?

@jaikiran
Copy link
Member Author

Tested over at https://github.com/greenbytes/java-http-1xx-tests and found to be working; thanks!

Thank you Julian for running those tests. Glad to hear it works now.

OpenJDK backports however are decided/managed by different team(s) and discussed in https://mail.openjdk.org/mailman/listinfo/jdk-updates-dev mailing list.

So is the proposal that I should follow up over there?

Yes, I suggest asking in that mailing list on how to get this backported. I would have given you a bit more precise instructions, but the last time I backported a change was way before the new bots based backporting was introduced, so I don't have more recent knowledge of backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
4 participants