8278067: Make HttpURLConnection default keep alive timeout configurable #437

dhanalla · 2024-02-07T16:54:16Z

The JDK11 patch didn't apply cleanly 1/4 hunks needed to be port manually.

Test performed and passed:

JTreg jdk_tier1, hotspot_tier1
Testcase attached this PR

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8326435 to be approved
Commit message must refer to an issue
JDK-8278067 needs maintainer approval

Issues

JDK-8278067: Make HttpURLConnection default keep alive timeout configurable (Enhancement - P4 - Approved)
JDK-8326435: Make HttpURLConnection default keep alive timeout configurable (CSR)

Reviewers

Andrew John Hughes (@gnu-andrew - Reviewer)
Severin Gehwolf (@jerboaa - Reviewer) ⚠️ Review applies to a78d9dc2

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 437

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-07T16:54:52Z

👋 Welcome back dhanalla! A progress list of the required criteria for merging this PR into pr/407 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-07T16:56:08Z

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

mlbridge · 2024-02-07T17:05:30Z

Webrevs

openjdk-notifier · 2024-02-08T10:25:30Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout Backport-JDK-8278067
git fetch https://git.openjdk.org/jdk8u-dev.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

jerboaa · 2024-02-08T10:56:23Z

Note the CSR for this is likely this one:
https://bugs.openjdk.org/browse/JDK-8288978

I've asked on the specifics of the 8u version, though.

dhanalla · 2024-02-08T17:36:21Z

Note the CSR for this is likely this one: https://bugs.openjdk.org/browse/JDK-8288978

I've asked on the specifics of the 8u version, though.

Thanks @jerboaa

dhanalla · 2024-02-20T18:20:06Z

Note the CSR for this is likely this one: https://bugs.openjdk.org/browse/JDK-8288978
I've asked on the specifics of the 8u version, though.

Thanks @jerboaa

@jerboaa I've added a follow-up request in the bug. In the meantime, could you please review this PR and the main PR #409 ?

dhanalla · 2024-02-21T22:28:19Z

Note the CSR for this is likely this one: https://bugs.openjdk.org/browse/JDK-8288978

I've asked on the specifics of the 8u version, though.

dhanalla · 2024-02-21T22:29:27Z

Note the CSR for this is likely this one: https://bugs.openjdk.org/browse/JDK-8288978
I've asked on the specifics of the 8u version, though.

@jerboaa, I have created a CSR https://bugs.openjdk.org/browse/JDK-8326435.

dhanalla · 2024-02-22T23:56:27Z

/csr needed

openjdk · 2024-02-22T23:56:54Z

@dhanalla has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@dhanalla please create a CSR request for issue JDK-8278067 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

gnu-andrew

The text in jdk/src/share/classes/java/net/doc-files/net-properties.html seems to be indented differently to the 11u version:

+--- a/jdk/src/share/classes/java/net/doc-files/net-properties.html
++++ b/jdk/src/share/classes/java/net/doc-files/net-properties.html
+@@ -172,6 +172,16 @@ <H2>Misc HTTP properties</H2>
        If HTTP keepalive is enabled (see above) this value determines the
        maximum number of idle connections that will be simultaneously kept
        alive, per destination.</P>
-+      <LI><P><B>{@systemProperty http.keepAlive.time.server}</B> and
-+        <B>{@systemProperty http.keepAlive.time.proxy}</B> </P>
++      <LI><P><B>http.keepAlive.time.server</B> and
++      <B>http.keepAlive.time.proxy</B> </P>
 +      <P>These properties modify the behavior of the HTTP keepalive cache in the case
-+        where the server (or proxy) has not specified a keepalive time. If the
-+        property is set in this case, then idle connections will be closed after the 
-+        specified number of seconds. If the property is set, and the server does
-+        specify a keepalive time in a "Keep-Alive" response header, then the time specified 
-+        by the server is used. If the property is not set and also the server
-+        does not specify a keepalive time, then connections are kept alive for an
-+        implementation defined time, assuming {@code http.keepAlive} is {@code true}.</P>
++      where the server (or proxy) has not specified a keepalive time. If the
++      property is set in this case, then idle connections will be closed after the
++      specified number of seconds. If the property is set, and the server does
++      specify a keepalive time in a "Keep-Alive" response header, then the time specified
++      by the server is used. If the property is not set and also the server
++      does not specify a keepalive time, then connections are kept alive for an
++      implementation defined time, assuming {@code http.keepAlive} is {@code true}.</P>
        <LI><P><B>http.maxRedirects</B> (default: 20)<BR>
        This integer value determines the maximum number, for a given request,
        of HTTP redirects that will be automatically followed by the

Can we please align these? Removing the @systemProperty tag should not mean changing the indentation of the entire block. Comparing while ignoring whitespace changes shows that most of it is identical, bar the first two lines.

Otherwise seems mostly clean. The change to the test library looks fine and the absence of the empty line removal is due to 8279842: HTTPS Channel Binding support for Java GSS/Kerberos being absent (which we should probably also backport at some point as it is in Oracle 8u341)

dhanalla · 2024-02-27T19:06:37Z

The text in jdk/src/share/classes/java/net/doc-files/net-properties.html seems to be indented differently to the 11u version:

+--- a/jdk/src/share/classes/java/net/doc-files/net-properties.html
++++ b/jdk/src/share/classes/java/net/doc-files/net-properties.html
+@@ -172,6 +172,16 @@ <H2>Misc HTTP properties</H2>
        If HTTP keepalive is enabled (see above) this value determines the
        maximum number of idle connections that will be simultaneously kept
        alive, per destination.</P>
-+      <LI><P><B>{@systemProperty http.keepAlive.time.server}</B> and
-+        <B>{@systemProperty http.keepAlive.time.proxy}</B> </P>
++      <LI><P><B>http.keepAlive.time.server</B> and
++      <B>http.keepAlive.time.proxy</B> </P>
 +      <P>These properties modify the behavior of the HTTP keepalive cache in the case
-+        where the server (or proxy) has not specified a keepalive time. If the
-+        property is set in this case, then idle connections will be closed after the 
-+        specified number of seconds. If the property is set, and the server does
-+        specify a keepalive time in a "Keep-Alive" response header, then the time specified 
-+        by the server is used. If the property is not set and also the server
-+        does not specify a keepalive time, then connections are kept alive for an
-+        implementation defined time, assuming {@code http.keepAlive} is {@code true}.</P>
++      where the server (or proxy) has not specified a keepalive time. If the
++      property is set in this case, then idle connections will be closed after the
++      specified number of seconds. If the property is set, and the server does
++      specify a keepalive time in a "Keep-Alive" response header, then the time specified
++      by the server is used. If the property is not set and also the server
++      does not specify a keepalive time, then connections are kept alive for an
++      implementation defined time, assuming {@code http.keepAlive} is {@code true}.</P>
        <LI><P><B>http.maxRedirects</B> (default: 20)<BR>
        This integer value determines the maximum number, for a given request,
        of HTTP redirects that will be automatically followed by the

Can we please align these? Removing the @systemProperty tag should not mean changing the indentation of the entire block. Comparing while ignoring whitespace changes shows that most of it is identical, bar the first two lines.

Otherwise seems mostly clean. The change to the test library looks fine and the absence of the empty line removal is due to 8279842: HTTPS Channel Binding support for Java GSS/Kerberos being absent (which we should probably also backport at some point as it is in Oracle 8u341)

Thanks @gnu-andrew for reviewing this PR.
Adjusted the Indentation according to 11u.

jerboaa

This seems fine to me. @gnu-andrew should ack it as well.

Nit: This tiny diff still makes it more different than it needs to be as compared to the 11u version (some trailing whitespace):

diff --git a/jdk/src/share/classes/java/net/doc-files/net-properties.html b/jdk/src/share/classes/java/net/doc-files/net-properties.html
index b1964ac4da..2d25e1b9f3 100644
--- a/jdk/src/share/classes/java/net/doc-files/net-properties.html
+++ b/jdk/src/share/classes/java/net/doc-files/net-properties.html
@@ -176,9 +176,9 @@ of proxies.</P>
         <B>http.keepAlive.time.proxy</B> </P>
        <P>These properties modify the behavior of the HTTP keepalive cache in the case
         where the server (or proxy) has not specified a keepalive time. If the
-        property is set in this case, then idle connections will be closed after the
+        property is set in this case, then idle connections will be closed after the 
         specified number of seconds. If the property is set, and the server does
-        specify a keepalive time in a "Keep-Alive" response header, then the time specified
+        specify a keepalive time in a "Keep-Alive" response header, then the time specified 
         by the server is used. If the property is not set and also the server
         does not specify a keepalive time, then connections are kept alive for an
         implementation defined time, assuming {@code http.keepAlive} is {@code true}.</P>

openjdk · 2024-03-06T13:21:36Z

⚠️ @dhanalla This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

gnu-andrew

Yes, the two lines Severin mentions still need to be fixed.
I recommend getting a local copy of the 11u patch - e.g. git show c9a72595aa2de5df5849d0b9458ed453bd0dfbad > /tmp/8278067.11u - and comparing that with what you have in your 8u repository. The only difference in the two patches for net-properties.html should be the http.keepAlive.time.server lines.

dhanalla · 2024-03-11T17:16:11Z

Yes, the two lines Severin mentions still need to be fixed. I recommend getting a local copy of the 11u patch - e.g. git show c9a72595aa2de5df5849d0b9458ed453bd0dfbad > /tmp/8278067.11u - and comparing that with what you have in your 8u repository. The only difference in the two patches for net-properties.html should be the http.keepAlive.time.server lines.

Thanks @gnu-andrew , Updated the trailing spaces to match those in 11u.

openjdk · 2024-03-13T20:05:15Z

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

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

8278067: Make HttpURLConnection default keep alive timeout configurable

Reviewed-by: andrew, 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 15 new commits pushed to the master branch:

147b418: 8327440: Fix "bad source file" error during beaninfo generation
fbb3392: 8325096: Test java/security/cert/CertPathBuilder/akiExt/AKISerialNumber.java is failing
f63643b: 8268916: Tests for AffirmTrust roots
1fad52d: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
d7ad5bf: 8159690: [TESTBUG] Mark headful tests with @key headful.
0bf84a2: 8198321: javax/swing/JEditorPane/5076514/bug5076514.java fails
0f20f67: 8326686: Bump update version of OpenJDK: 8u422
f05ea98: 8240756: [macos] SwingSet2:TableDemo:Printed Japanese characters were garbled
a798523: 8025439: [TEST BUG] [macosx] PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105
824dbfb: 8186199: [windows] JNI_DestroyJavaVM not covered by SEH
... and 5 more: https://git.openjdk.org/jdk8u-dev/compare/9bd600c8ac096afd05c1633a5e56ee2a359aa35c...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 (@gnu-andrew, @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).

gnu-andrew

Thanks for persisting with this. Updated version looks good.

jerboaa · 2024-03-18T17:24:13Z

GHA failures are https://bugs.openjdk.org/browse/JDK-8324583

gnu-andrew · 2024-03-19T13:55:10Z

/approve yes

openjdk · 2024-03-19T13:56:23Z

@gnu-andrew
8278067: The approval request has been approved.

dhanalla · 2024-03-19T16:07:37Z

/integrate

openjdk · 2024-03-19T16:08:20Z

@dhanalla
Your change (at version 10ab2a4) is now ready to be sponsored by a Committer.

jerboaa · 2024-03-19T16:27:01Z

/sponsor

openjdk · 2024-03-19T16:28:20Z

Going to push as commit 6ba9f58.
Since your change was applied there have been 15 commits pushed to the master branch:

147b418: 8327440: Fix "bad source file" error during beaninfo generation
fbb3392: 8325096: Test java/security/cert/CertPathBuilder/akiExt/AKISerialNumber.java is failing
f63643b: 8268916: Tests for AffirmTrust roots
1fad52d: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
d7ad5bf: 8159690: [TESTBUG] Mark headful tests with @key headful.
0bf84a2: 8198321: javax/swing/JEditorPane/5076514/bug5076514.java fails
0f20f67: 8326686: Bump update version of OpenJDK: 8u422
f05ea98: 8240756: [macos] SwingSet2:TableDemo:Printed Japanese characters were garbled
a798523: 8025439: [TEST BUG] [macosx] PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105
824dbfb: 8186199: [windows] JNI_DestroyJavaVM not covered by SEH
... and 5 more: https://git.openjdk.org/jdk8u-dev/compare/9bd600c8ac096afd05c1633a5e56ee2a359aa35c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-19T16:28:33Z

@jerboaa @dhanalla Pushed as commit 6ba9f58.

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

dhanalla added 2 commits December 15, 2023 18:21

Backport of 2e31cc7ee1b875af7c7b3a5367ac8056fbc60650

0ef34e3

Backport d8f44aa39e921594505864e6270f42b745265293

80387af

openjdk bot changed the title ~~Backport d8f44aa39e921594505864e6270f42b745265293~~ 8278067: Make HttpURLConnection default keep alive timeout configurable Feb 7, 2024

openjdk bot added backport rfr Pull request is ready for review labels Feb 7, 2024

Fix indentation

22fb611

openjdk-notifier bot changed the base branch from pr/407 to master February 8, 2024 10:25

openjdk bot mentioned this pull request Feb 8, 2024

8293562: KeepAliveCache Blocks Threads while Closing Connections #409

Closed

4 tasks

dhanalla and others added 2 commits February 20, 2024 23:56

Remove SystemProperty tag as it is not supported in JDK8

f44e27b

Merge branch 'master' into Backport-JDK-8278067

4aa85b8

openjdk bot mentioned this pull request Feb 21, 2024

8291226: Create Test Cases to cover scenarios for JDK-8278067 #453

Closed

4 tasks

dhanalla closed this Feb 21, 2024

dhanalla reopened this Feb 21, 2024

openjdk bot added the csr Pull request needs approved CSR before integration label Feb 22, 2024

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 23, 2024

gnu-andrew reviewed Feb 27, 2024

View reviewed changes

Adjusting the indentation according to 11u.

a78d9dc

openjdk bot mentioned this pull request Mar 4, 2024

8291637: HttpClient default keep alive timeout not followed if server sends invalid value #466

Closed

4 tasks

jerboaa approved these changes Mar 6, 2024

View reviewed changes

openjdk bot added the approval label Mar 6, 2024

gnu-andrew reviewed Mar 9, 2024

View reviewed changes

Add trailing spaces to make it consistent with 11u

10ab2a4

gnu-andrew approved these changes Mar 18, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed approval labels Mar 19, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 19, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 19, 2024

openjdk bot closed this Mar 19, 2024

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 Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8278067: Make HttpURLConnection default keep alive timeout configurable #437

8278067: Make HttpURLConnection default keep alive timeout configurable #437

dhanalla commented Feb 7, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

mlbridge bot commented Feb 7, 2024 •

edited

Loading

openjdk-notifier bot commented Feb 8, 2024

jerboaa commented Feb 8, 2024

dhanalla commented Feb 8, 2024

dhanalla commented Feb 20, 2024

dhanalla commented Feb 21, 2024

dhanalla commented Feb 21, 2024

dhanalla commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

gnu-andrew left a comment

dhanalla commented Feb 27, 2024

jerboaa left a comment

openjdk bot commented Mar 6, 2024

gnu-andrew left a comment

dhanalla commented Mar 11, 2024

openjdk bot commented Mar 13, 2024 •

edited

Loading

gnu-andrew left a comment

jerboaa commented Mar 18, 2024

gnu-andrew commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

dhanalla commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

jerboaa commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

8278067: Make HttpURLConnection default keep alive timeout configurable #437

8278067: Make HttpURLConnection default keep alive timeout configurable #437

Conversation

dhanalla commented Feb 7, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

mlbridge bot commented Feb 7, 2024 • edited Loading

Webrevs

openjdk-notifier bot commented Feb 8, 2024

jerboaa commented Feb 8, 2024

dhanalla commented Feb 8, 2024

dhanalla commented Feb 20, 2024

dhanalla commented Feb 21, 2024

dhanalla commented Feb 21, 2024

dhanalla commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

gnu-andrew left a comment

Choose a reason for hiding this comment

dhanalla commented Feb 27, 2024

jerboaa left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 6, 2024

gnu-andrew left a comment

Choose a reason for hiding this comment

dhanalla commented Mar 11, 2024

openjdk bot commented Mar 13, 2024 • edited Loading

gnu-andrew left a comment

Choose a reason for hiding this comment

jerboaa commented Mar 18, 2024

gnu-andrew commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

dhanalla commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

jerboaa commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

dhanalla commented Feb 7, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading