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: set default timeout in agent keepSocketAlive #33127

Closed

Conversation

omsmith
Copy link
Contributor

@omsmith omsmith commented Apr 28, 2020

Previous location of setting the timeout would override behaviour of custom HttpAgents' keepSocketAlive. Moving it into the default keepSocketAlive allows it to interoperate with custom agents.

Fixes: #33111

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Previous location of setting the timeout would override behaviour of custom HttpAgents' keepSocketAlive. Moving it into the default keepSocketAlive allows it to interoperate with custom agents.

Fixes: nodejs#33111
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 28, 2020
@omsmith
Copy link
Contributor Author

omsmith commented Apr 28, 2020

/cc @ronag

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Apr 30, 2020

Landed in 0413acc

@ronag ronag closed this Apr 30, 2020
ronag pushed a commit that referenced this pull request Apr 30, 2020
Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: #33111

PR-URL: #33127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@omsmith
Copy link
Contributor Author

omsmith commented May 2, 2020

Since f9123eb91b was included in 12.16.3, can I suggest this get a backport to 12.x?

targos pushed a commit that referenced this pull request May 4, 2020
Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: #33111

PR-URL: #33127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@targos targos mentioned this pull request May 4, 2020
ronag pushed a commit to nxtedition/node that referenced this pull request May 10, 2020
Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: nodejs#33111

PR-URL: nodejs#33127
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@omsmith omsmith deleted the custom-agent-keepalive-timeout branch May 31, 2020 15:36
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"max socket lifetime" Agent impacted by v13.11 socket timeout changes
8 participants