FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: added scheduling option to http agent by delvedor · Pull Request #33278 · nodejs/node · GitHub

/ node Public

http: added scheduling option to http agent - #33278

Closed
delvedor wants to merge 2 commits into
nodejs:masterfrom
delvedor:lifo-agent
Closed

http: added scheduling option to http agent#33278
delvedor wants to merge 2 commits into
nodejs:masterfrom
delvedor:lifo-agent

Conversation

delvedor commented May 7, 2020
edited
Loading

Copy link
Copy Markdown
Member

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 added the http Issues or PRs related to the http subsystem. label May 7, 2020

ronag commented May 7, 2020

Copy link
Copy Markdown
Member

Please update docs

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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
Comment thread doc/api/http.md Outdated
delvedor requested a review from mcollina May 7, 2020 12:55

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label May 7, 2020
Comment thread doc/api/http.md Outdated

jasnell May 7, 2020
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No hard block!

Added scheduling http agent option.
mcollina added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 17, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

I think this can land

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member

Landed in daa65fb

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 <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
QwireyInc pushed a commit to QwireyInc/node that referenced this pull request Apr 19, 2025
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>
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
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 outstanding review comments, 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


Back | FazBrowse Home | New Git URL