http: added scheduling option to http agent #33278

delvedor · 2020-05-07T08:53:27Z

Hello everyone!

This pr introduces a new configuration option in the HTTP agent, named scheduling.
By default, the agent iterates over the free sockets in a FIFO fashion, the newly added scheduling option allows using a LIFO instead.
If nothing is configured, the behavior will be the same as before, so this change should not be considered as breaking.

Why this is useful?

I've encountered a case where the application was sending requests at a high rate (around 200 req/sec), with low response time. Every now and then, the response time increased massively for a few seconds and the agent started opening as many sockets as it could, later, the response time was low again, and only a few sockets were used at a given time.
The issue is that the agent will not close the freeSockets, because since the default scheduling sequence is FIFO, every socket will be reused before it reaches its timeout.
This situation was happening in a system with multiple instances of Node, and the receiver of the request was overwhelmed by the high number of open and never closed sockets.

By switching to a LIFO scheduling, I saw that the spikes were handled as expected, but as soon as the response time was low again, the freeSockets start decreasing, and only the minimum amount of sockets was used.

Given that both scheduling strategies have pros and cons, I've decided to expose them as an option, so the user can have more control based on their needs.

How does it work?

The user only need to configure the scheduling option, only fifo and lifo are accepted.

const agent = new http.Agent({
  keepAlive: true,
  maxSockets: 256,
  maxFreeSockets: 256,
  timeout: 30000,
  scheduling: 'lifo'
})

I didn't update the documentation yet, I'll do that once we settle on the feature and the implementation :)
If possible, I would love to see this change landing both in Node.js v12 and v14.

Related: #31526
cc @mcollina @ronag

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

In some cases, it is preferable to use a lifo scheduling strategy for the free sockets instead of default one, which is fifo. This commit introduces a scheduling option to add the ability to choose which strategy best fits your needs.

ronag · 2020-05-07T09:26:26Z

Please update docs

mcollina

Adding a "request changes" only because it's missing docs.

doc/api/http.md

mcollina

lgtm

jasnell · 2020-05-07T21:56:35Z

doc/api/http.md

@@ -132,6 +137,9 @@ added: v0.3.4
  * `maxFreeSockets` {number} Maximum number of sockets to leave open
    in a free state. Only relevant if `keepAlive` is set to `true`.
    **Default:** `256`.
+  * `scheduling` {string} Scheduling strategy to apply when picking
+    the next free socket to use. It can be `'fifo'` or `'lifo'`.
+    **Default:** `'fifo'`


Non-blocking: Would be ideal to have fifo and lifo briefly explained as not all readers may be familiar with the terms. Might also be good to briefly explain why one would stray from the default.

Thank you for the suggestion! Let me know what you think :)

jasnell · 2020-05-07T22:02:47Z

test/parallel/test-http-agent-scheduling.js

+      maxSockets: 5
+    });
+
+    bulkRequest(url, agent, (ports) => {


Might be a bit nicer to convert these into async functions with await syntax and to wrap this stack into a helper function so it can be reused in both tests.

Thank you for the suggestion! Unfortunately, I don't have time for refactoring this test at the moment, is it ok if we leave it as is?

@jasnell is not clear if you have an hard block on the tests being converted or not. Can we land this anyway?

No hard block!

Added scheduling http agent option.

mcollina

lgtm

I think this can land

nodejs-github-bot · 2020-05-19T22:03:45Z

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

mcollina · 2020-05-20T09:00:59Z

Landed in daa65fb

In some cases, it is preferable to use a lifo scheduling strategy for the free sockets instead of default one, which is fifo. This commit introduces a scheduling option to add the ability to choose which strategy best fits your needs. PR-URL: #33278 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In some cases, it is preferable to use a lifo scheduling strategy for the free sockets instead of default one, which is fifo. This commit introduces a scheduling option to add the ability to choose which strategy best fits your needs. PR-URL: nodejs/node#33278 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http: added scheduling option to http agent

fd4dd0a

In some cases, it is preferable to use a lifo scheduling strategy for the free sockets instead of default one, which is fifo. This commit introduces a scheduling option to add the ability to choose which strategy best fits your needs.

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

ronag approved these changes May 7, 2020

View reviewed changes

mcollina requested changes May 7, 2020

View reviewed changes

mcollina added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v10.x labels May 7, 2020

delvedor requested review from mcollina and ronag May 7, 2020 10:09

mcollina reviewed May 7, 2020

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

delvedor force-pushed the lifo-agent branch from ee44444 to 063c55f Compare May 7, 2020 12:55

delvedor requested a review from mcollina May 7, 2020 12:55

mcollina approved these changes May 7, 2020

View reviewed changes

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label May 7, 2020

jasnell reviewed May 7, 2020

View reviewed changes

ronag approved these changes May 7, 2020

View reviewed changes

jasnell reviewed May 7, 2020

View reviewed changes

http: updated http agent documentation

6230e94

Added scheduling http agent option.

delvedor force-pushed the lifo-agent branch from 063c55f to 6230e94 Compare May 8, 2020 09:04

mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2020

mcollina approved these changes May 17, 2020

View reviewed changes

mcollina closed this May 20, 2020

mcollina mentioned this pull request May 20, 2020

http: Change free sockets behavior to LIFO from FIFO. #31526

Closed

2 tasks

dkMorlok mentioned this pull request Jun 27, 2020

algorithm: freeSockets pop/shift node-modules/agentkeepalive#8

Open

This was referenced Jul 11, 2021

chore(deps): update node.js to >=v12.22.3 kufu/smarthr-ui#1741

Closed

chore(deps): update node.js -> >=v12.22.12 - engines brodycj/react-native-module-init#125

Open

gary-kim-bot mentioned this pull request Aug 2, 2021

chore(deps): update node.js to >=v12.22.4 gary-kim/nc-build-script#63

Open

1 task

renovate bot mentioned this pull request Aug 3, 2021

Update Node.js to >=v12.22.4 Pytal/deploy-env#89

Merged

1 task

This was referenced Aug 11, 2021

Update Node.js to v12.22.5 SirReiva/backend-mongo-express-jwt#48

Closed

Update Node.js to v12.22.12 shaimael-ws-java-demo/mariadb#41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: added scheduling option to http agent #33278

http: added scheduling option to http agent #33278

delvedor commented May 7, 2020 •

edited

Loading

ronag commented May 7, 2020

mcollina left a comment

mcollina left a comment

jasnell May 7, 2020 •

edited

Loading

delvedor May 8, 2020

jasnell May 7, 2020

delvedor May 8, 2020

mcollina May 17, 2020

jasnell May 17, 2020

mcollina left a comment

nodejs-github-bot commented May 19, 2020

mcollina commented May 20, 2020

http: added scheduling option to http agent #33278

http: added scheduling option to http agent #33278

Conversation

delvedor commented May 7, 2020 • edited Loading

Why this is useful?

How does it work?

Checklist

ronag commented May 7, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

jasnell May 7, 2020 • edited Loading

Choose a reason for hiding this comment

delvedor May 8, 2020

Choose a reason for hiding this comment

jasnell May 7, 2020

Choose a reason for hiding this comment

delvedor May 8, 2020

Choose a reason for hiding this comment

mcollina May 17, 2020

Choose a reason for hiding this comment

jasnell May 17, 2020

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 19, 2020

mcollina commented May 20, 2020

delvedor commented May 7, 2020 •

edited

Loading

jasnell May 7, 2020 •

edited

Loading