-
Notifications
You must be signed in to change notification settings - Fork 162
Improve HTTP connection handling #1180
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
Open
sg70
wants to merge
1
commit into
jfrog:master
Choose a base branch
from
sg70:improve-http-connection-handling
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
e72b560 to
1d32ab7
Compare
Author
|
I have read the CLA Document and I hereby sign the CLA. |
1d32ab7 to
e0d2202
Compare
Author
|
@ehl-jf can have a look on this PR? |
69baf32 to
08a05ea
Compare
08a05ea to
578ebc0
Compare
578ebc0 to
57d08c2
Compare
57d08c2 to
713cf5e
Compare
713cf5e to
a21cd9e
Compare
a21cd9e to
7db32a0
Compare
- stop closing TCP connections after each HTTP request - add ForceAttemptHTTP2: true as parameter for http.Transport - increase MaxIdleConnsPerHost to 6
7db32a0 to
aaebde5
Compare
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.

Closes jfrog/jfrog-cli-artifactory#290
What does this MR do?
This MR enables the underlying
http.clientto reuse HTTP connections.Why was this MR needed?
The relevant part of the HTTP client implementation has remained unchanged since 2017. It is unclear why
req.Closewas set totrue, which forces the Gohttp.Clientto close the connection immediately after the response is received. Reusing established HTTP(S) connections is a common best practice and can significantly improve performance.The function
createDefaultHttpTransportinhttp/httpclient/clientBuilder.gouses the same defaults as the originalDefaultTransportimplementation in thenet/httppackage. SinceForceAttemptHTTP2was added as a default in thenet/httppackage after 2017, I added this option as well.After performance testing, I set the parameter
MaxIdleConnsPerHostto6, aligning with modern web browsers’ common default and providing a balanced improvement in connection reuse, especially when HTTP/2 is not used. The previous default of 2 resulted in a high number of short-lived TCP connections under multi-threaded workloads.Performance test case
Download a directory from Artifactory with 4591 files ranging from 100 Bytes to 21 MByte (total size 892.13 MByte) with a max connection throughput of 350 Mbit/s. Tests were conducted on the internet where the Artifactory server is hosted at AWS behind an AWS ALB. Client and AWS region are in the same country. The
jfrog-cliwas used with its default configuration. All test were executed multiple times.current jfrog-cli release (2.78.10)
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 3: 221 secs
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 10 : 97 secs
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 20 : 69 secs
with the change of the PR applied:
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 3: 124 secs
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 10 : 64 secs
Download as directory using jfrog-cli with options --include-dirs --quiet --threads 20 : 46 secs
During the test runs with the change of the PR applied, the number of TCP connections was stable, and TCP connections got reused. Furthermore the total amount of open TCP connections when using HTTP2 was not more than 4 connections.
Potential Drawbacks
While reusing connections generally improves performance, it's important to note that the Go
http.Clienthas set aIdleConnTimeoutof 90 seconds. This means that idle connections will be closed after 90 seconds of inactivity, preventing them from becoming stale indefinitely. Therefore, the risk of long-lived connections causing issues is mitigated by this timeout. The benefits of connection reuse are expected to outweigh any potential drawbacks in most common scenarios.