Skip to content

Use Keep-Alive by default in global agents and close idle connections in server#43522

Closed
ShogunPanda wants to merge 1 commit into
nodejs:mainfrom
ShogunPanda:default-agent
Closed

Use Keep-Alive by default in global agents and close idle connections in server#43522
ShogunPanda wants to merge 1 commit into
nodejs:mainfrom
ShogunPanda:default-agent

Conversation

@ShogunPanda

@ShogunPanda ShogunPanda commented Jun 21, 2022

Copy link
Copy Markdown
Contributor

This PR introduces the following changes:

  1. Both http.globalAgent and https.globalAgent use keep-alive by default.
  2. Agent now parses and applies Keep-Alive headers received by the server (if smaller than the current agent timeout)
  3. http(s).Server.close now calls closeIdleConnections internally.

Fixes: #37184

@ShogunPanda ShogunPanda requested review from mcollina and ronag June 21, 2022 12:59
@nodejs-github-bot

nodejs-github-bot commented Jun 21, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. labels Jun 21, 2022
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 21, 2022
mcollina
mcollina approved these changes Jun 21, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

mcollina commented Jun 23, 2022

Copy link
Copy Markdown
Member

cc @nodejs/tsc

also @mscdex

aduh95
aduh95 approved these changes Jun 23, 2022
View reviewed changes
Comment thread lib/_http_agent.js Outdated
@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44887/

mcollina
mcollina approved these changes Jun 27, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

cc @nodejs/tsc this needs another approval

@Trott

Trott commented Jun 27, 2022

Copy link
Copy Markdown
Member

cc @nodejs/tsc this needs another approval

It has two TSC approvals already (you and Antoine), so I think it's good as far as our landing-a-semver-major rules go. (However, I wouldn't discourage someone else from reviewing!)

@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44898/

@Trott

Trott commented Jun 27, 2022

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2964/

@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44903/

@github-actions github-actions Bot mentioned this pull request Jun 28, 2022
Open
32 tasks
@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44935/

@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44938/

@github-actions github-actions Bot mentioned this pull request Jun 29, 2022
Open
23 tasks
@ShogunPanda

ShogunPanda commented Jun 29, 2022

Copy link
Copy Markdown
Contributor Author

Landed in 4267b92

ShogunPanda added a commit that referenced this pull request Jun 29, 2022
@ShogunPanda
http: use Keep-Alive by default in global agents
4267b92 
PR-URL: #43522
Fixes: #37184
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ShogunPanda ShogunPanda deleted the default-agent branch June 29, 2022 10:49
@MoLow MoLow mentioned this pull request Jun 30, 2022
Closed
@tniessen tniessen mentioned this pull request Jul 1, 2022
Merged
@F3n67u F3n67u mentioned this pull request Jul 1, 2022
Closed
@RafaelGSS RafaelGSS mentioned this pull request Oct 17, 2022
Closed
@mcollina mcollina mentioned this pull request Oct 21, 2022
Merged
4 tasks
@richardbann richardbann mentioned this pull request Nov 15, 2022
Closed
@Linkgoron Linkgoron mentioned this pull request Jun 7, 2023
Merged
@trevor-scheer trevor-scheer mentioned this pull request Jul 21, 2023
Merged
@himself65 himself65 mentioned this pull request Oct 15, 2023
Closed
@BrandonGillis BrandonGillis mentioned this pull request Dec 6, 2023
Merged
5 tasks
@ShogunPanda ShogunPanda mentioned this pull request Dec 14, 2023
Closed
@lpinca lpinca mentioned this pull request Jan 12, 2024
Closed
1 task
@fahrradflucht fahrradflucht mentioned this pull request Apr 6, 2024
Merged
@nikwen nikwen mentioned this pull request Aug 11, 2024
Closed
@jishi jishi mentioned this pull request Nov 21, 2024
Closed
@ArtskydJ ArtskydJ mentioned this pull request Dec 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@aduh95 aduh95 aduh95 approved these changes

@ronag ronag Awaiting requested review from ronag

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Set a default (http|https).Agent with keep-alive

5 participants

@ShogunPanda @nodejs-github-bot @mcollina @Trott @aduh95