8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure by c-cleary · Pull Request #12457 · openjdk/jdk

c-cleary · 2023-02-07T15:41:23Z

Description

Intermittent failures of this test are observed on frequent HttpClient test runs. The test checks that the same connection is not used twice for two seperate requests if an Idle Connection Timeout occurs by verifying that the client-side port does not use the same port. It also verifies that when an Idle Connection Timeout does not occur, the same connection is used by verifying that the port used in both requests is the same.

The issue here is that there is no guarantee that the ports used will not be the same for when an Idle Connection Timeout occurs and so the test will/does fail intermittently.

Summary of Changes & Justification

Instead of comparing the post numbers of the connections used for each request in all test cases, the connections themselves are now compared with calls to hashCode() like so. The connection instances themselves are accessed by using a customised ExchangeSupplier for the Http2TestServer.

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-8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12457

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

Using diff file

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

…ermittent failure

bridgekeeper · 2023-02-07T15:42:16Z

👋 Welcome back ccleary! 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 · 2023-02-07T15:44:15Z

@c-cleary 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.

mlbridge · 2023-02-07T15:52:38Z

Webrevs

01: Full - Incremental (f125c27c)
00: Full (99913369)

dfuch · 2023-02-07T16:08:53Z

test/jdk/java/net/httpclient/http2/IdleConnectionTimeoutTest.java

+                if (!acceptedFirstConnection) {
+                    firstConnectionHash = exch.getTestConnection().hashCode();
+                    acceptedFirstConnection = true;
+                    exch.sendResponseHeaders(200, 0);
+                } else {
+                    int secondConnectionHash = exch.getTestConnection().hashCode();
+                    int cmp = Integer.compare(firstConnectionHash, secondConnectionHash);
+
+                    if (cmp < 0 | cmp > 0) {


In other words, if (cmp != 0) ? ;-)

It may be simpler and less error prone to save the connection (as an Object) in the handler.

volatile Object previousConnection = null;

Then the condition become:

if (previousConnection == null) { previousConnection = exch.getTestConnection(); exch.sendResponseHeaders(200, 0); } else { var secondConnection = exch.getTestConnection(); if (previousConnection != secondConnection) { ....

I could be wrong but I think I might have tried that before. What you're calling previousConnection in the sample code you've given gets set to null in the time between the first and second requests during the tests where a timeout fires. Which does serve to test that the connections are different to be fair but maybe for some reason previousConnection could be null that is unrelated to it being closed by the test server. For example createConnection() might close early due to a Reset Stream or Protocol Error.

I know that's very edge case but its the reason I compared using the hash code. I'm open to changing it though of course! Might be less error prone as you say.

Also, very fair on the if (cmp != 0) suggestion 😅 I'll definitely shorten it to that if I stick with the comparison as it is right now.

I don't see how using the hash wouldn't cause the exact same issue. Also you really need volatile here, since two different requests are going to be handled by two different threads.

c-cleary · 2023-02-10T08:34:35Z

Changes were simplified, original means of obtaining the hash code of each connection was abandoned in favour of just comparing the object references as @dfuch suggested.

dfuch

LGTM! Thanks for making these changes.

openjdk · 2023-02-10T10:47:19Z

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

8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure

Reviewed-by: dfuchs

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

c8ace48: 8301072: Replace NULL with nullptr in share/oops/
1c7b09b: 8302069: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java update
837d464: 8302125: Make G1 full gc abort the VM after failing VerifyDuringGC verification
723433d: 8302117: IgnoreUnrecognizedVMOptions flag causes failure in ArchiveHeapTestClass
e245620: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange()
b814cfc: 8178806: Better exception logging in crypto code
8c87a67: 8245654: Add Certigna Root CA
97d0c87: 8302109: Trivial fixes to btree tests
0aeebee: 8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap
4815566: 8228604: StackMapFrames are missing from redefined class bytes of retransformed classes
... and 49 more: https://git.openjdk.org/jdk/compare/9dad874ff9f03f5891aa8b37e7826a67c851f06d...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.

c-cleary · 2023-02-14T09:39:13Z

As this was a minor change and testing seems to be consistent, I'll proceed with integration

c-cleary · 2023-02-14T09:39:50Z

/integrate

openjdk · 2023-02-14T09:41:45Z

Going to push as commit 92474f1.
Since your change was applied there have been 99 commits pushed to the master branch:

ee5f6e1: 8302337: JDK crashes if lib/modules contains non-zero byte containing ATTRIBUTE_END
7f71a10: 8301874: BarrierSetC2 should assign barrier data to stores
d782125: 8302214: Typo in javadoc of Arrays.compare and Arrays.mismatch
94e7cc8: 8301226: Add clamp() methods to java.lang.Math and to StrictMath
13b1ebb: 8298853: JvmtiVTMSTransitionDisabler should support disabling one virtual thread transitions
abbeb7e: 8302108: Clean up placeholder supername code
d503c66: 8302155: [AIX] NUM_LCPU is not required variable
c37e9d1: 8298293: NMT: os::realloc() should verify that flags do not change between reallocations
101db26: 8301697: [s390] Optimized-build is broken
f4d4fa5: 8300255: Introduce interface for GC oop verification in the assembler
... and 89 more: https://git.openjdk.org/jdk/compare/9dad874ff9f03f5891aa8b37e7826a67c851f06d...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-14T09:42:07Z

@c-cleary Pushed as commit 92474f1.

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

8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java int…

9991336

…ermittent failure

openjdk bot added the net net-dev@openjdk.org label Feb 7, 2023

c-cleary marked this pull request as ready for review February 7, 2023 15:47

openjdk bot added the rfr Pull request is ready for review label Feb 7, 2023

dfuch reviewed Feb 7, 2023

View reviewed changes

8301243: Simplified changes to server handlers

f125c27

dfuch approved these changes Feb 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 10, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 14, 2023

openjdk bot closed this Feb 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure#12457

8301243: java/net/httpclient/http2/IdleConnectionTimeoutTest.java intermittent failure#12457
c-cleary wants to merge 2 commits intoopenjdk:masterfrom
c-cleary:JDK-8301243

c-cleary commented Feb 7, 2023 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Feb 7, 2023

Uh oh!

openjdk bot commented Feb 7, 2023

Uh oh!

mlbridge bot commented Feb 7, 2023 •

edited

Loading

Uh oh!

dfuch Feb 7, 2023

Uh oh!

c-cleary Feb 7, 2023 •

edited

Loading

Uh oh!

dfuch Feb 7, 2023

Uh oh!

c-cleary commented Feb 10, 2023

Uh oh!

dfuch left a comment

Uh oh!

openjdk bot commented Feb 10, 2023

Uh oh!

c-cleary commented Feb 14, 2023

Uh oh!

c-cleary commented Feb 14, 2023

Uh oh!

openjdk bot commented Feb 14, 2023

Uh oh!

openjdk bot commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

c-cleary commented Feb 7, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary of Changes & Justification

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 7, 2023

Uh oh!

openjdk bot commented Feb 7, 2023

Uh oh!

mlbridge bot commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

c-cleary Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

c-cleary commented Feb 10, 2023

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 10, 2023

Uh oh!

c-cleary commented Feb 14, 2023

Uh oh!

c-cleary commented Feb 14, 2023

Uh oh!

openjdk bot commented Feb 14, 2023

Uh oh!

openjdk bot commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

c-cleary commented Feb 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2023 •

edited

Loading

c-cleary Feb 7, 2023 •

edited

Loading