8293562: KeepAliveCache Blocks Threads while Closing Connections #1890

PoojaDP-23 · 2023-05-22T04:54:31Z

fix: https://bugs.openjdk.org/browse/JDK-8293562

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-8293562: KeepAliveCache Blocks Threads while Closing Connections (Bug - P4)

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1890

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

Using diff file

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

Webrev

Link to Webrev Comment

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

bridgekeeper · 2023-05-22T04:55:22Z

👋 Welcome back PoojaDP-23! 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.

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

mlbridge · 2023-05-22T12:12:21Z

Webrevs

jerboaa · 2023-05-22T12:37:42Z

Seems a duplicate of #1825

alograg · 2023-05-24T13:28:55Z

Hi @PoojaDP-23,
I was working on the backporting of the fix. #1825
But after the @jerboaa reviews, I realized that your code is closer to the original fix commit than the mine.
I offer you all my support, I remain at your disposal.

jerboaa · 2023-05-24T14:17:02Z

@PoojaDP-23 Please use Backport 03f25a9c6924430ec4063b801b2b6ca55b9067c9 as the PR title so that bots recognize it as a backport. Also, please enable Github Action testing on your fork.

PoojaDP-23 · 2023-05-25T10:15:54Z

Hi @PoojaDP-23, I was working on the backporting of the fix. #1825 But after the @jerboaa reviews, I realized that your code is closer to the original fix commit than the mine. I offer you all my support, I remain at your disposal.

Thanks

openjdk · 2023-05-25T10:17:48Z

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

jerboaa · 2023-05-25T10:20:26Z

@PoojaDP-23 Please also merge latest master tree into the PR branch so as to trigger GHA checks for this PR. Thanks!

PoojaDP-23 · 2023-05-25T10:28:27Z

@jerboaa - Yes, I have enabled Github Action testing on my fork

PoojaDP-23 · 2023-05-30T10:50:28Z

@jerboaa - I have performed below steps to merge latest master into PR branch. Could you please take a look?

git checkout master
git pull https://github.com/openjdk/jdk11u-dev master
git checkout <Your_BRANCH_NAME>
git merge master
git push origin HEAD

alograg · 2023-06-09T08:07:46Z

Hi @jerboaa
Can you do the review ?
Thrnaks,

jerboaa

This seems OK to me. A few minor comments. What testing have you done?

jerboaa · 2023-06-13T09:14:39Z

src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java

@@ -268,8 +290,8 @@ private void readObject(ObjectInputStream stream)
    }
 }

-/* FILO order for recycling HttpClients, should run in a thread
- * to time them out.  If > maxConns are in use, block.
+ /* LIFO order for reusing HttpClients. Most recent entries at the front.


Nit: Extraneous space in front of /*.

jerboaa · 2023-06-13T09:18:47Z

test/jdk/sun/net/www/http/KeepAliveCache/B8293562.java

+                            "Wait for second request was interrupted"));
+                }
+            } else {
+                System.out.println("name of the thread" +threadName);


This isn't part of the original change. Please remove.

jerboaa · 2023-06-13T09:19:28Z

test/jdk/sun/net/www/http/KeepAliveCache/B8293562.java

+                        "Close invoked from unexpected thread"));
+            }
+            System.out.println("Connection closed");
+            System.out.println(threadName);


Same here. Extraneous debug println()?

darinhoward · 2023-06-15T20:54:30Z

The docs at https://www.oracle.com/java/technologies/javase/11-0-17-relnotes.html indicate this fix was done in 11.0.17.0.3. I'm assuming this is in error based on this PR still being open.

alograg · 2023-06-16T08:27:22Z

Hi @darinhoward
The bug is fixed in version JDK 11.0.17.0.3 (Oracle) not in OpenJDK 11.0.17

jerboaa · 2023-06-16T08:57:47Z

The docs at https://www.oracle.com/java/technologies/javase/11-0-17-relnotes.html indicate this fix was done in 11.0.17.0.3. I'm assuming this is in error based on this PR still being open.

Yes, those are Oracle JDK release notes. This PR is for OpenJDK.

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

PoojaDP-23 · 2023-06-19T07:31:37Z

@jerboaa - I have addressed review comments and I have enabled GHA actions to test and I have separately run the testcase "B8293562.java" with(passes) and without(fails) the fix to test these changes. Thanks

openjdk · 2023-06-19T08:38:46Z

@PoojaDP-23 This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8293562: KeepAliveCache Blocks Threads while Closing Connections

Reviewed-by: sgehwolf

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

2cc4596: 8293858: Change PKCS7 code to use default SecureRandom impl instead of SHA1PRNG
c1f759e: 8305815: Update Libpng to 1.6.39
d981db8: 8255348: NPE in PKIXCertPathValidator event logging code
d5d3981: 8297523: Various GetPrimitiveArrayCritical miss result - NULL check
0b7efe4: 8023980: JCE doesn't provide any class to handle RSA private key in PKCS#1
09685c8: 8155246: Throw error if default java.security file is missing
c1939dd: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes
d7701cd: 8242897: KeyFactory.generatePublic( x509Spec ) failed with java.security.InvalidKeyException
ad283ae: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML
c1bb926: 8309138: Fix container tests for jdks with symlinked conf dir
... and 29 more: https://git.openjdk.org/jdk11u-dev/compare/9eded681616aa0f926001dc93e4f397512ad0cd4...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 (@jerboaa) 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).

PoojaDP-23 · 2023-06-19T09:17:24Z

/integrate

openjdk · 2023-06-19T09:19:28Z

@PoojaDP-23
Your change (at version 0e9c050) is now ready to be sponsored by a Committer.

jerboaa · 2023-06-19T12:29:59Z

@PoojaDP-23 Please only integrate once you have approval from maintainers for this release. See Step 6 in https://wiki.openjdk.org/display/JDKUpdates/How+to+contribute+or+backport+a+fix

PoojaDP-23 · 2023-06-21T06:54:14Z

/label

openjdk · 2023-06-21T06:54:41Z

@PoojaDP-23 Usage: /label <add|remove> [label[, label, ...]] or /label [<+|->label[, <+|->label, ...]] where label is an additional classification that should be applied to this PR. These labels are valid:

PoojaDP-23 · 2023-06-22T04:46:44Z

@jerboaa - Could you please add the fix request to the jbs issue? Please let me know if there are any actions from my side. Thanks

jerboaa · 2023-06-22T08:29:51Z

@jerboaa - Could you please add the fix request to the jbs issue? Please let me know if there are any actions from my side. Thanks

Done. Please mention what extra testing of the patch you've done here (other than the regtest). Thanks!

GoeLin · 2023-06-22T10:19:41Z

Hi @PoojaDP-23
You should not integrate before you have the fix-yes label!!!

I assume JDK-8296438, which is listed as related issue, is a follow up.

jerboaa · 2023-06-22T12:18:11Z

Please check https://bugs.openjdk.org/browse/JDK-8296943 before this gets sponsored.

PoojaDP-23 · 2023-06-27T10:35:42Z

@GoeLin - Apologies for commenting integrate. @jerboaa - I have enabled GHA actions and also I have ran the TIER1 and TIER2 tests and all are passing and I have ran the jtreg test test/jdk/sun/net/www/http/KeepAliveCache/B8293562.java which passes with this fix and fails without the fix
TIER: 1
Test summary

   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/hotspot/jtreg:tier1                     1482  1482     0     0
   jtreg:test/jdk:tier1                               1881  1881     0     0
   jtreg:test/langtools:tier1                         3934  3934     0     0
   jtreg:test/nashorn:tier1                              0     0     0     0
   jtreg:test/jaxp:tier1                                 0     0     0     0

TEST SUCCESS

TIER: 2
Test summary

 TEST                                              TOTAL  PASS  FAIL ERROR
 jtreg:test/hotspot/jtreg:tier2                      325   325     0     0
 jtreg:test/jdk:tier2                               3468  3468     0     0
 jtreg:test/langtools:tier2                           11    11     0     0
 jtreg:test/nashorn:tier2                             36    36     0     0
 jtreg:test/jaxp:tier2                               440   440     0     0

TEST SUCCESS

PoojaDP-23 · 2023-06-27T11:24:45Z

@jerboaa - I have checked https://bugs.openjdk.org/browse/JDK-8296943 and I have validated the test sun/net/www/http/HttpClient/MultiThreadTest.java with this fix and test is passing. Please take a look on below screenshot.

Java-version:

Test-result

jerboaa · 2023-06-27T11:35:07Z

OK, thanks!

jerboaa · 2023-06-27T11:36:04Z

/sponsor

openjdk · 2023-06-27T11:37:46Z

Going to push as commit 770c1f6.
Since your change was applied there have been 70 commits pushed to the master branch:

ccbb928: 8260878: com/sun/jdi/JdbOptions.java fails without jfr
193dc76: 8259796: timed CompletableFuture.get may swallow InterruptedException
97c07a1: 8234808: jdb quoted option parsing broken
5506ecf: 8254350: CompletableFuture.get may swallow InterruptedException
cc88f4c: 8303511: C2: assert(get_ctrl(n) == cle_out) during unrolling
4dd85cb: 8297587: Upgrade JLine to 3.22.0
e0b98bc: 8292033: Move jdk.X509Certificate event logic to JCA layer
b2e04df: 8289508: Improve test coverage for XPath Axes: ancestor, ancestor-or-self, preceding, and preceding-sibling
7f1047e: 8297955: LDAP CertStore should use LdapName and not String for DNs
4a7557e: 8236045: [TESTBUG] MismatchedWhiteBox test fails with missing WhiteBox$WhiteBoxPermission.class
... and 60 more: https://git.openjdk.org/jdk11u-dev/compare/9eded681616aa0f926001dc93e4f397512ad0cd4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-27T11:37:56Z

@jerboaa @PoojaDP-23 Pushed as commit 770c1f6.

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

Pooja-DP added 2 commits May 12, 2023 00:10

Backport 454ef9ba2b2b15250a29251556f79a0464df5ccc

ed9eda5

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

Address indentation

42ea817

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

address whitespace error

15db90f

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

PoojaDP-23 changed the title ~~KeepAliveCache Blocks Threads while Closing Connections.~~ JDK-8293562: KeepAliveCache Blocks Threads while Closing Connections. May 22, 2023

openjdk bot added the rfr Pull request is ready for review label May 22, 2023

jerboaa mentioned this pull request May 24, 2023

8293562: KeepAliveCache Blocks Threads while Closing Connections #1825

Closed

3 tasks

PoojaDP-23 changed the title ~~JDK-8293562: KeepAliveCache Blocks Threads while Closing Connections.~~ Backport 03f25a9c6924430ec4063b801b2b6ca55b9067c9 May 25, 2023

openjdk bot changed the title ~~Backport 03f25a9c6924430ec4063b801b2b6ca55b9067c9~~ 8293562: KeepAliveCache Blocks Threads while Closing Connections May 25, 2023

openjdk bot added the backport label May 25, 2023

Merge branch 'master' into keep-alive

0a7a3fd

jerboaa reviewed Jun 13, 2023

View reviewed changes

address review comments

0e9c050

Signed-off-by: Pooja.D.P <Pooja.D.P1@ibm.com>

jerboaa approved these changes Jun 19, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 19, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 19, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 27, 2023

openjdk bot closed this Jun 27, 2023

openjdk bot 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 Jun 27, 2023

This was referenced Dec 12, 2023

8293562: KeepAliveCache Blocks Threads while Closing Connections openjdk/jdk8u-dev#402

Closed

8293562: KeepAliveCache Blocks Threads while Closing Connections openjdk/jdk8u-dev#409

Closed

8293562: KeepAliveCache Blocks Threads while Closing Connections #1890

8293562: KeepAliveCache Blocks Threads while Closing Connections #1890

Conversation

PoojaDP-23 commented May 22, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 22, 2023

mlbridge bot commented May 22, 2023 • edited Loading

Webrevs

jerboaa commented May 22, 2023

alograg commented May 24, 2023

jerboaa commented May 24, 2023

PoojaDP-23 commented May 25, 2023

openjdk bot commented May 25, 2023

jerboaa commented May 25, 2023

PoojaDP-23 commented May 25, 2023

PoojaDP-23 commented May 30, 2023 • edited Loading

alograg commented Jun 9, 2023

jerboaa left a comment

Choose a reason for hiding this comment

jerboaa Jun 13, 2023

Choose a reason for hiding this comment

jerboaa Jun 13, 2023

Choose a reason for hiding this comment

jerboaa Jun 13, 2023

Choose a reason for hiding this comment

darinhoward commented Jun 15, 2023 • edited by bridgekeeper bot Loading

alograg commented Jun 16, 2023

jerboaa commented Jun 16, 2023

PoojaDP-23 commented Jun 19, 2023

openjdk bot commented Jun 19, 2023

PoojaDP-23 commented Jun 19, 2023

openjdk bot commented Jun 19, 2023

jerboaa commented Jun 19, 2023

PoojaDP-23 commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

PoojaDP-23 commented Jun 22, 2023

jerboaa commented Jun 22, 2023

GoeLin commented Jun 22, 2023

jerboaa commented Jun 22, 2023

PoojaDP-23 commented Jun 27, 2023

PoojaDP-23 commented Jun 27, 2023

jerboaa commented Jun 27, 2023

jerboaa commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

PoojaDP-23 commented May 22, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 22, 2023 •

edited

Loading

PoojaDP-23 commented May 30, 2023 •

edited

Loading

darinhoward commented Jun 15, 2023 •

edited by bridgekeeper bot

Loading