http: set lifo as the default scheduling strategy in Agent. #36685

mcollina · 2020-12-30T12:05:49Z

In #33278 we have added a scheduling option for our http.Agent to use a LIFO algorithm instead of FIFO. This greatly reduces the amount of ECONNRESET errors due to timeouts, and it's generically a better production experience for our users.

I propose to make this the default.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mcollina · 2020-12-30T12:06:22Z

@nodejs/tsc can you please review?

addaleax · 2020-12-30T12:14:52Z

I’d personally also be happy to land this as a semver-minor change.

mcollina · 2020-12-30T12:19:31Z

I’d personally also be happy to land this as a semver-minor change.

I'm definitely ok with this. Would you backport down to v14?

mcollina · 2020-12-30T18:56:04Z

cc @delvedor

delvedor

100% agree!
It should not be a semver major as the socket handling is transparent to the user :)

BridgeAR

LGTM % an update to the scheduling documentation. It is still documented that fifo is the default :).

Changing it as semver-minor sounds good to me as well!

nodejs-github-bot · 2021-01-05T21:15:34Z

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

github-actions · 2021-01-05T22:39:31Z

Commit Queue failed

- Loading data for nodejs/node/pull/36685 ✔ Done loading data for nodejs/node/pull/36685 ----------------------------------- PR info ------------------------------------ Title http: set lifo as the default scheduling strategy in Agent. (#36685) Author Matteo Collina (@mcollina) Branch mcollina:default-lifo -> nodejs:master Labels baking-for-lts, dont-land-on-v10.x, dont-land-on-v12.x, http, lts-watch-v14.x, semver-minor Commits 1 - http: set lifo as the default scheduling strategy in Agent. Committers 1 - Matteo Collina PR-URL: https://github.com/nodejs/node/pull/36685 Reviewed-By: Anna Henningsen Reviewed-By: Michaël Zasso Reviewed-By: Robert Nagy Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Daijiro Wachi Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36685 Reviewed-By: Anna Henningsen Reviewed-By: Michaël Zasso Reviewed-By: Robert Nagy Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Daijiro Wachi Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2021-01-05T21:15:34Z: https://ci.nodejs.org/job/node-test-pull-request/35312/ - Querying data for job/node-test-pull-request/35312/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Wed, 30 Dec 2020 12:05:49 GMT ✔ Approvals: 8 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/36685#pullrequestreview-560024151 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36685#pullrequestreview-560024681 ✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36685#pullrequestreview-560034670 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/36685#pullrequestreview-560059651 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36685#pullrequestreview-560175591 ✔ - Daijiro Wachi (@watilde): https://github.com/nodejs/node/pull/36685#pullrequestreview-560418195 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/36685#pullrequestreview-560564331 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36685#pullrequestreview-560647420 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 36685 From https://github.com/nodejs/node * branch refs/pull/36685/merge -> FETCH_HEAD ✔ Fetched commits as a3fcf24a6c2f..2b8c2f649209 -------------------------------------------------------------------------------- Auto-merging lib/_http_agent.js Auto-merging doc/api/http.md [master 83957d65c9] http: set lifo as the default scheduling strategy in Agent. Author: Matteo Collina Date: Wed Dec 30 13:02:27 2020 +0100 3 files changed, 8 insertions(+), 5 deletions(-) ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- http: set lifo as the default scheduling strategy in Agent.

PR-URL: #36685
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Michaël Zasso targos@protonmail.com
Reviewed-By: Robert Nagy ronagy@icloud.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Daijiro Wachi daijiro.wachi@gmail.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de
Reviewed-By: Rich Trott rtrott@gmail.com

[master 9a83efb9ce] http: set lifo as the default scheduling strategy in Agent.
Author: Matteo Collina hello@matteocollina.com
Date: Wed Dec 30 13:02:27 2020 +0100
3 files changed, 8 insertions(+), 5 deletions(-)
✖ 9a83efb9ce62f0772bf6dfa471dc8a35181adfc3
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
⚠ 0:50 Title should be <= 50 columns. title-length
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✖ 0:59 Do not use punctuation at end of title. title-format

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/464594830

mcollina · 2021-01-06T10:19:20Z

Landed in 278c91c

PR-URL: #36685 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412) * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603) * crypto: * implement basic secure heap support (James M Snell) [#36779](#36779) * fixup bug in keygen error handling (James M Snell) [#36779](#36779) * introduce X509Certificate API (James M Snell) [#36804](#36804) * implement randomuuid (James M Snell) [#36729](#36729) * doc: * update release key for Danielle Adams (Danielle Adams) [#36793](#36793) * add dnlup to collaborators (Daniele Belardi) [#36849](#36849) * add panva to collaborators (Filip Skokan) [#36802](#36802) * add yashLadha to collaborator (Yash Ladha) [#36666](#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291) * v8: * fix native constructors (ExE Boss) [#36549](#36549)

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) (#29412) * support AbortSignal in fork (Benjamin Gruenbaum) (#36603) * crypto: * implement basic secure heap support (James M Snell) (#36779) * fixup bug in keygen error handling (James M Snell) (#36779) * introduce X509Certificate API (James M Snell) (#36804) * implement randomuuid (James M Snell) (#36729) * doc: * update release key for Danielle Adams (Danielle Adams) (#36793) * add dnlup to collaborators (Daniele Belardi) (#36849) * add panva to collaborators (Filip Skokan) (#36802) * add yashLadha to collaborator (Yash Ladha) (#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) (#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) (#34291) * v8: * fix native constructors (ExE Boss) (#36549)

mcollina · 2021-04-14T10:58:04Z

@nodejs/releasers could you add this to the next v14 release?

targos · 2021-04-14T11:21:13Z

@mcollina I cherry-picked it on v14.x-staging: f70aee0

PR-URL: #36685 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 30, 2020

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 30, 2020

mcollina requested review from lpinca, lundibundi and ronag December 30, 2020 12:05

mcollina requested review from addaleax and jasnell December 30, 2020 12:06

addaleax approved these changes Dec 30, 2020

View reviewed changes

targos approved these changes Dec 30, 2020

View reviewed changes

mcollina force-pushed the default-lifo branch from 5bd5025 to 2ff264e Compare December 30, 2020 12:20

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020

ronag approved these changes Dec 30, 2020

View reviewed changes

jasnell approved these changes Dec 30, 2020

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020

lpinca approved these changes Dec 30, 2020

View reviewed changes

delvedor approved these changes Dec 30, 2020

View reviewed changes

watilde approved these changes Dec 31, 2020

View reviewed changes

BridgeAR approved these changes Jan 1, 2021

View reviewed changes

http: set lifo as the default scheduling strategy in Agent.

2b8c2f6

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021

mmarchini added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 5, 2021

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 5, 2021

mcollina closed this Jan 6, 2021

mcollina deleted the default-lifo branch January 8, 2021 09:35

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos removed baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 14, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

styfle mentioned this pull request Jul 22, 2021

Add keepAlive to node-fetch polyfill vercel/next.js#27376

Merged

targos removed the lts-watch-v14.x label Sep 4, 2021

mcollina mentioned this pull request Sep 15, 2021

Follow curl: Enable setNoDelay() / TCP_NODELAY by default on HTTP(S) 1.x #34185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: set lifo as the default scheduling strategy in Agent. #36685

http: set lifo as the default scheduling strategy in Agent. #36685

mcollina commented Dec 30, 2020

mcollina commented Dec 30, 2020

addaleax commented Dec 30, 2020

mcollina commented Dec 30, 2020

mcollina commented Dec 30, 2020

delvedor left a comment

BridgeAR left a comment •

edited

Loading

nodejs-github-bot commented Jan 5, 2021

github-actions bot commented Jan 5, 2021

mcollina commented Jan 6, 2021

mcollina commented Apr 14, 2021

targos commented Apr 14, 2021

http: set lifo as the default scheduling strategy in Agent. #36685

http: set lifo as the default scheduling strategy in Agent. #36685

Conversation

mcollina commented Dec 30, 2020

Checklist

mcollina commented Dec 30, 2020

addaleax commented Dec 30, 2020

mcollina commented Dec 30, 2020

mcollina commented Dec 30, 2020

delvedor left a comment

Choose a reason for hiding this comment

BridgeAR left a comment • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 5, 2021

github-actions bot commented Jan 5, 2021

mcollina commented Jan 6, 2021

mcollina commented Apr 14, 2021

targos commented Apr 14, 2021

BridgeAR left a comment •

edited

Loading