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

8297030: Reduce Default Keep-Alive Timeout Value for httpclient #11285

Closed
wants to merge 1 commit into from

Conversation

c-cleary
Copy link
Contributor

@c-cleary c-cleary commented Nov 22, 2022

Proposed changes to reduce the default Keep Alive Timeout value in jdk/internal/net/http/HttpClientImpl.java from 1200 seconds to 30 seconds. The current default value of 1200s is needlessly high and in remote connections that allow a client to suggest a value, this will mean that idle connections will wait very long before being dropped.

This value of 30 seconds was chosen by reviewing the current default values for Keep-Alive timeouts in common server implementations and choosing a value that would have the client's default value be lower whenever the value can be set.

Further discussion on this change can be seen the pull request 8288717: Add a means to close idle connections in HTTP/2 connection pool #10183.


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
  • Change requires a CSR request to be approved

Issues

  • JDK-8297030: Reduce Default Keep-Alive Timeout Value for httpclient
  • JDK-8297558: Reduce Default Keep-Alive Timeout Value for httpclient (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11285

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 22, 2022

👋 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 openjdk bot added the rfr Pull request is ready for review label Nov 22, 2022
@openjdk
Copy link

openjdk bot commented Nov 22, 2022

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

@openjdk openjdk bot added the net net-dev@openjdk.org label Nov 22, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 22, 2022

Webrevs

@c-cleary
Copy link
Contributor Author

c-cleary commented Nov 22, 2022

Some data on default Keep-Alive values for servers...

  • Apache default Keepalive timeout is 15s
  • AWS CloudFront default idle connection timeout value is 5-60s
  • Load Balancers, tend to have longer defaults. Probably due to queing/access-control systems preparing for high demmand.
    • Azure minimum is 240s
    • AWS ELB, default of 350s

This data is hard to come by without digging into sources for the above but it is clear that our default should at the very least be lower than present.

@c-cleary
Copy link
Contributor Author

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Nov 22, 2022
@openjdk
Copy link

openjdk bot commented Nov 22, 2022

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

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

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

I agree that 30s seems a much better default value than 1200s. Would be good to get @Michael-Mc-Mahon opinion too. This change will have to be coordinated with https://git.openjdk.org/jdk/pull/11241: Depending on which PR gets integrated first, you may have to update module-info (or leave it to Michael)

@Michael-Mc-Mahon
Copy link
Member

30 seconds sounds good.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 28, 2022
@openjdk
Copy link

openjdk bot commented Nov 28, 2022

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

8297030: Reduce Default Keep-Alive Timeout Value for httpclient

Reviewed-by: dfuchs, michaelm, jpai

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

  • a249a52: 8296754: AutoCreateSharedArchive in JDK 20 is not compatible with JDK 19
  • 405b188: 8297570: jdk/jfr/threading/TestDeepVirtualStackTrace.java fails with -XX:-UseTLAB
  • ba0a252: 8297717: Remove jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java from ProblemList
  • c05dc80: 8297660: x86: Redundant test+jump in C1 allocateArray
  • eff4c03: 8297343: TestStress*.java fail with "got different traces for the same seed"
  • 6a856bc: 8297499: Parallel: Missing iteration over klass when marking objArrays/objArrayOops during Full GC
  • b80f5af: 8297309: Memory leak in ShenandoahFullGC
  • 81eb5fb: 8297534: Specify the size of MEMFLAGS
  • 012dafe: 8297082: Remove sun/tools/jhsdb/BasicLauncherTest.java from problem list
  • 43d1173: 8286302: Port JEP 425 to PPC64
  • ... and 61 more: https://git.openjdk.org/jdk/compare/42c2037429a8ee6f683bbbc99fb48c540519524c...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 28, 2022
@c-cleary
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 29, 2022

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

  • 1301fb0: 8296470: Refactor VMError::report STEP macro to improve readability
  • 48017b1: 8296804: Document HttpClient configuration properties in java.net.http module-info
  • bd51b7e: 8296645: org.openjdk.bench.javax.tools.Javac leaves class files in current directory
  • b27a61e: 8297216: Search results omit some methods
  • 33dfc7d: 8296954: G1: Enable parallel scanning for heap region remset
  • 6f06f44: 8297242: Use-after-free during library unloading on Linux
  • 4e8e853: 6788481: CellEditorListener.editingCanceled is never called
  • 692bedb: 8297106: Remove the -Xcheck:jni local reference capacity checking
  • 05128c2: 8286185: The Java manpage can be more platform inclusive
  • d450314: 8297276: Remove thread text from Subject.current
  • ... and 74 more: https://git.openjdk.org/jdk/compare/42c2037429a8ee6f683bbbc99fb48c540519524c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 29, 2022
@openjdk openjdk bot closed this Nov 29, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 29, 2022
@openjdk
Copy link

openjdk bot commented Nov 29, 2022

@c-cleary Pushed as commit 42b60ed.

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

@fantasyzh
Copy link

fantasyzh commented Feb 19, 2024

Why httpclient not honor server returned "Keep-Alive: timeout=N" header?
HttpURLConnection will use this header's
timeout value if header exists。

@dfuch
Copy link
Member

dfuch commented Feb 19, 2024

This was never implemented. Note that KeepAlive is only valid for HTTP/1.1 - not HTTP/2
See https://datatracker.ietf.org/doc/html/rfc9113#section-8.2.2.

I have logged an RFE https://bugs.openjdk.org/browse/JDK-8326160

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