8278067: Make HttpURLConnection default keep alive timeout configurable #7349

Michael-Mc-Mahon · 2022-02-04T13:11:02Z

Hi,

Could I get the following patch reviewed please? (A CSR is also required which I will submit when the docs are agreed)

It adds a pair of new system properties to make the keep alive timer in java.net.HttpURLConnection configurable.
The proposed property names are:

"http.keepAlive.time.server" and "http.keepAlive.time.proxy"

Thanks,
Michael

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8278067: Make HttpURLConnection default keep alive timeout configurable
JDK-8281828: Make HttpURLConnection default keep alive timeout configurable (CSR)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7349

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

Using diff file

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

Michael-Mc-Mahon · 2022-02-04T13:11:27Z

/csr needed

bridgekeeper · 2022-02-04T13:12:27Z

👋 Welcome back michaelm! 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 · 2022-02-04T13:14:10Z

@Michael-Mc-Mahon has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@Michael-Mc-Mahon please create a CSR request for issue JDK-8278067. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2022-02-04T13:15:53Z

@Michael-Mc-Mahon 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 · 2022-02-04T13:19:25Z

Webrevs

dfuch · 2022-02-04T14:11:49Z

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

-                v = new ClientVector(keepAliveTimeout > 0 ?
-                        keepAliveTimeout * 1000 : LIFETIME);
+                if (keepAliveTimeout == 0) {
+                    keepAliveTimeout = getUserKeepAlive(http.getUsingProxy());


This could be 0 if -Dhttp.keepAlive.time.xxx=0 was specified which would result in an assert below.
Also I am not sure I understand the logic of having same 5s timeout for server & proxy if nothing was specified anywhere, but having a different value for proxy & server if the server specified keep-alive without providing a value.
Where does that come from?

Ah, I meant to catch that case as well in the method where the property is read. Probably should be documented as well.

The other behavior is just maintaining the present behavior, purely for compatibility reasons.

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

dfuch

Good catch! I'd missed that.

openjdk · 2022-02-16T05:47:21Z

@Michael-Mc-Mahon 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:

8278067: Make HttpURLConnection default keep alive timeout configurable

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

0af356b: 8278173: [vectorapi] Add x64 intrinsics for unsigned (zero extended) casts
a24498b: 8281771: Crash in java_lang_invoke_MethodType::print_signature
1aff44b: 8279949: JavaThread::_free_handle_block leaks native memory
394ce5f: 8280825: Modules that "provide" ToolProvider should document the name that can be used
745f7e7: 8281186: runtime/cds/appcds/DumpingWithNoCoops.java fails
1870465: 8281744: x86: Use short jumps in TIG::set_vtos_entry_points
2fe0bf6: 8281748: runtime/logging/RedefineClasses.java failed "assert(addr != __null) failed: invariant"
bc61484: 8280136: Serial: Remove unnecessary use of ExpandHeap_lock
2112a9d: 8246033: bin/print_config.js script uses nashorn jjs tool
1c12b15: 8281741: [testbug] PrintIdealPhaseTest fails with -Xcomp
... and 14 more: https://git.openjdk.java.net/jdk/compare/95f198b2b1b2d5437515dc837cc160e4224c0ff3...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.

Michael-Mc-Mahon · 2022-02-16T16:00:12Z

/integrate

openjdk · 2022-02-16T16:01:23Z

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

7428b37: 8281948: JFR: Parser skips too many bytes for fractional types
d5b4666: 8281829: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java fails after JDK-8281467
fef5d74: 8281812: x86: Use short jumps in TemplateTable::condy_helper
a86cab8: 8236136: tests which use CompilationMode shouldn't be run w/ TieredStopAtLevel
0af356b: 8278173: [vectorapi] Add x64 intrinsics for unsigned (zero extended) casts
a24498b: 8281771: Crash in java_lang_invoke_MethodType::print_signature
1aff44b: 8279949: JavaThread::_free_handle_block leaks native memory
394ce5f: 8280825: Modules that "provide" ToolProvider should document the name that can be used
745f7e7: 8281186: runtime/cds/appcds/DumpingWithNoCoops.java fails
1870465: 8281744: x86: Use short jumps in TIG::set_vtos_entry_points
... and 18 more: https://git.openjdk.java.net/jdk/compare/95f198b2b1b2d5437515dc837cc160e4224c0ff3...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-16T16:01:47Z

@Michael-Mc-Mahon Pushed as commit d8f44aa.

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

Michael-Mc-Mahon added 9 commits December 6, 2021 10:11

initial work

f9ded24

Merge branch 'master' into keepalive

16398cb

Merge branch 'master' into keepalive

7d1e6be

Merge branch 'master' into keepalive

41968d2

Merge branch 'master' into keepalive

23c9f2f

Merge branch 'master' into keepalive

afff251

updates

b78cef3

reverted change to LIFETIME constant. Were not necessary

d1a2ee1

added docs

85d5662

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Feb 4, 2022

openjdk bot added the net net-dev@openjdk.org label Feb 4, 2022

dfuch reviewed Feb 4, 2022

View reviewed changes

Michael-Mc-Mahon added 3 commits February 11, 2022 12:48

Merge branch 'master' into keepalive

a46e8b0

update after Daniel's first review

edcbb13

Merge branch 'master' into keepalive

b0b7673

dfuch reviewed Feb 14, 2022

View reviewed changes

test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java Show resolved Hide resolved

test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java Outdated Show resolved Hide resolved

Fixed test case problem and update from Daniel's review

79f64a7

dfuch approved these changes Feb 15, 2022

View reviewed changes

tighten up spec wording and move props beside existing keep alive ones

9a7a4ce

dfuch approved these changes Feb 15, 2022

View reviewed changes

remove extraneous .swp file

d047808

dfuch approved these changes Feb 15, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 16, 2022

openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2022

openjdk bot added the integrated Pull request has been integrated label Feb 16, 2022

openjdk bot closed this Feb 16, 2022

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

Michael-Mc-Mahon deleted the keepalive branch February 16, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8278067: Make HttpURLConnection default keep alive timeout configurable #7349

8278067: Make HttpURLConnection default keep alive timeout configurable #7349

Michael-Mc-Mahon commented Feb 4, 2022 •

edited by openjdk bot

Loading

Michael-Mc-Mahon commented Feb 4, 2022

bridgekeeper bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

mlbridge bot commented Feb 4, 2022 •

edited

Loading

dfuch Feb 4, 2022 •

edited

Loading

Michael-Mc-Mahon Feb 4, 2022

dfuch left a comment

openjdk bot commented Feb 16, 2022 •

edited

Loading

Michael-Mc-Mahon commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

8278067: Make HttpURLConnection default keep alive timeout configurable #7349

8278067: Make HttpURLConnection default keep alive timeout configurable #7349

Conversation

Michael-Mc-Mahon commented Feb 4, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Michael-Mc-Mahon commented Feb 4, 2022

bridgekeeper bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

mlbridge bot commented Feb 4, 2022 • edited Loading

Webrevs

dfuch Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Michael-Mc-Mahon Feb 4, 2022

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2022 • edited Loading

Michael-Mc-Mahon commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

Michael-Mc-Mahon commented Feb 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 4, 2022 •

edited

Loading

dfuch Feb 4, 2022 •

edited

Loading

openjdk bot commented Feb 16, 2022 •

edited

Loading