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

8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST #5964

Closed

Conversation

evwhelan
Copy link
Contributor

@evwhelan evwhelan commented Oct 15, 2021

Hi,

Please review my fix for JDK-8274779 which changes how HttpClient and HttpsClient checks for equality when comparing request methods.

When HttpURLConnection.setRequestMethod is passed new String("POST") rather than the "POST" String literal, the old behaviour resulted in broken HttpClients being reused from the KeepAliveCache.

This is because a call to HttpClient.available() was never reachable due to identity equality being used instead of logical equality.

The test case uses an injected KeepAliveCache, to which we put a HttpClient that is unavailable. By comparing the initial HttpClient's connectTimeout value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we can assert that these values should never be equal as a new HttpClient should be created in cases where we can no longer use the cached one.

All CI testing is green for this fix.

Kind regards,
Evan


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
$ git checkout pull/5964

Update a local copy of the PR:
$ git checkout pull/5964
$ git pull https://git.openjdk.java.net/jdk pull/5964/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5964

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5964.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 15, 2021

👋 Welcome back ewhelan! 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 Oct 15, 2021
@openjdk
Copy link

openjdk bot commented Oct 15, 2021

@evwhelan 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 Oct 15, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 15, 2021

Webrevs

@evwhelan evwhelan changed the title 8274779: HttpClient and HttpsClient incorrectly check request method when set to POST 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST Oct 15, 2021
Copy link
Member

@dfuch dfuch left a comment

Code changes look good. I am a bit more suspicious of the test.

* @library /test/lib
* @modules java.base/sun.net.www.http:+open
* java.base/sun.net.www.protocol.http
* @run testng RequestMethodEquality
Copy link
Member

@dfuch dfuch Oct 15, 2021

Choose a reason for hiding this comment

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

This test modifies some static fields in HttpClient so should run in /othervm

Copy link
Contributor Author

@evwhelan evwhelan Oct 15, 2021

Choose a reason for hiding this comment

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

Fixed thanks!

// Injecting a mock KeepAliveCache that the HttpClient can use
KeepAliveCache kac = new KeepAliveCache();
kac.put(url, null, freshClient);
Copy link
Member

@dfuch dfuch Oct 15, 2021

Choose a reason for hiding this comment

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

Instead of injecting a new KeepAliveCache you could get the cache already present in HttpClient. One way to do this is to use reflection - another is to inject an accessor class in the sun.net.www.http package. You may not even need to have to create a "fresh" HttpClient - there should be one there corresponding to the connection you already created?

Copy link
Contributor Author

@evwhelan evwhelan Oct 15, 2021

Choose a reason for hiding this comment

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

Thanks Daniel! I've used an injected accessor instead like you've suggested.

In regards to creating a "fresh" client. From looking at the implementation of HttpURLConnection, the internal HttpClient (namely, http) is not actually initialised during this test. There are various methods in which it gets initialised (setNewClient, proxiedConnect, plainConnect0). With the way I've written this test, none of these methods are called and the internal http object remains null throughout.

This is why I am explicitly creating a fresh and cached client. By setting the differing connectTimeout values and comparing them, I feel it nicely demonstrates whether or not the same client is being used, while trying to minimise the amount of actual network communication that gets carried out.

I verified the test behaviour before and after the source fix and before, the cachedClient was always being initialised with the broken client we manually inserted into the cache

@openjdk
Copy link

openjdk bot commented Oct 15, 2021

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

8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST

Reviewed-by: dfuchs, coffeys, vtewari, 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 93 new commits pushed to the master branch:

  • 0c3eaea: 8168388: GetMousePositionTest fails with the message "Mouse position should not be null"
  • 09f5235: 8275405: Linking error for classes with lambda template parameters and virtual functions
  • a120937: 8274988: G1: refine G1SegmentedArrayAllocOptions and G1CardSetAllocOptions
  • c7a80e6: 8275607: G1: G1CardSetAllocator::drop_all needs to call G1SegmentedArray::drop_all
  • af7c56b: 8275167: x86 intrinsic for unsignedMultiplyHigh
  • cea3f01: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
  • d1e3ca4: 8233558: [TESTBUG] WindowOwnedByEmbeddedFrameTest.java fails on macos
  • 913f928: 8273507: Convert test/jdk/java/nio/channels/Channels/TransferTo.java to TestNG test
  • 0021a2f: 8275449: Add linux-aarch64-zero build profile
  • 46b5bfb: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/4cb7124c1e9c5fd1d3a82fd8933cc63fefde9531...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @coffeys, @vyommani, @Michael-Mc-Mahon) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2021
@Michael-Mc-Mahon
Copy link
Member

Michael-Mc-Mahon commented Oct 15, 2021

Just wondering what actually happens if you encounter this bug? I guess that the post will fail somehow because it has been given a closed connection. If so, would it be possible to write the test to simulate that behavior?

So, say you initially do a HTTP request that succeeds (creating a cached connection). Then, somehow close the connection independent of the keep alive cache. Then attempt the streaming post which will either succeed or fail depending whether the bug is fixed or not.

Field inCache = HttpClient.class.getDeclaredField("inCache");
inCache.setAccessible(true);
inCache.setBoolean(freshClient, true); // allows the assertion in HttpClient.New to pass
Copy link
Member

@dfuch dfuch Oct 15, 2021

Choose a reason for hiding this comment

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

You can use the HttpClientAccess to get and set this field too, since the field is protected in HttpClient.
Just add a method:

public void setInCache(HttpClient client, boolean inCache) { client.inCache = inCache; } 

to HttpClientAccess.

Copy link
Contributor Author

@evwhelan evwhelan Oct 19, 2021

Choose a reason for hiding this comment

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

You're right, made that change! Thanks

* rather than the "POST" String literal
* @bug 8274779
* @library /test/lib
* @modules java.base/sun.net.www.http:+open
Copy link
Member

@dfuch dfuch Oct 19, 2021

Choose a reason for hiding this comment

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

I think that now you can probably remove :+open - since you're no longer using reflection.

Copy link
Contributor Author

@evwhelan evwhelan Oct 20, 2021

Choose a reason for hiding this comment

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

Done, thanks!

Copy link
Contributor

@vyommani vyommani left a comment

Looks good to me.

@Michael-Mc-Mahon
Copy link
Member

Michael-Mc-Mahon commented Oct 20, 2021

Just wondering what actually happens if you encounter this bug? I guess that the post will fail somehow because it has been given a closed connection. If so, would it be possible to write the test to simulate that behavior?

So, say you initially do a HTTP request that succeeds (creating a cached connection). Then, somehow close the connection independent of the keep alive cache. Then attempt the streaming post which will either succeed or fail depending whether the bug is fixed or not.

We discussed this off line, and it doesn't seem to be feasible. I'm happy with the fix as is

dfuch
dfuch approved these changes Oct 20, 2021
@evwhelan evwhelan force-pushed the Incorrect_POST_Equality_Check branch from f5b0d2d to 1325792 Compare Oct 20, 2021
@evwhelan
Copy link
Contributor Author

evwhelan commented Oct 20, 2021

The force push was just modifying the previous git commit message. Including an ampersand accidentally tagged a github user with the username "modules"

@evwhelan
Copy link
Contributor Author

evwhelan commented Oct 21, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 21, 2021
@openjdk
Copy link

openjdk bot commented Oct 21, 2021

@evwhelan
Your change (at version 1325792) is now ready to be sponsored by a Committer.

@coffeys
Copy link
Contributor

coffeys commented Oct 21, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 21, 2021

Going to push as commit 45ce06c.
Since your change was applied there have been 97 commits pushed to the master branch:

  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • cd07b3c: 8257534: misc tests failed with "NoClassDefFoundError: Could not initialize class java.util.concurrent.ThreadLocalRandom"
  • 819d2df: 8274794: Print all owned locks in hs_err file
  • c41ce6d: 8275415: Prepare Leak Profiler for Lilliput
  • 0c3eaea: 8168388: GetMousePositionTest fails with the message "Mouse position should not be null"
  • 09f5235: 8275405: Linking error for classes with lambda template parameters and virtual functions
  • a120937: 8274988: G1: refine G1SegmentedArrayAllocOptions and G1CardSetAllocOptions
  • c7a80e6: 8275607: G1: G1CardSetAllocator::drop_all needs to call G1SegmentedArray::drop_all
  • af7c56b: 8275167: x86 intrinsic for unsignedMultiplyHigh
  • cea3f01: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
  • ... and 87 more: https://git.openjdk.java.net/jdk/compare/4cb7124c1e9c5fd1d3a82fd8933cc63fefde9531...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 21, 2021
@openjdk
Copy link

openjdk bot commented Oct 21, 2021

@coffeys @evwhelan Pushed as commit 45ce06c.

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

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
5 participants