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

http: added scheduling option to http agent #33278

Closed
wants to merge 2 commits into from

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented May 7, 2020

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.
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 7, 2020
@ronag
Copy link
Member

ronag commented May 7, 2020

Please update docs

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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

@mcollina 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 delvedor requested review from mcollina and ronag May 7, 2020 10:09
doc/api/http.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label May 7, 2020
doc/api/http.md Outdated
@@ -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'`
Copy link
Member

@jasnell jasnell May 7, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

maxSockets: 5
});

bulkRequest(url, agent, (ports) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

No hard block!

Added scheduling http agent option.
@mcollina 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
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I think this can land

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

Landed in daa65fb

@mcollina mcollina closed this May 20, 2020
mcollina pushed a commit that referenced this pull request May 20, 2020
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
codebytere pushed a commit that referenced this pull request May 21, 2020
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants