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.

Sign up for GitHub

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

Jump to bottom

http: destroy timeout socket by Agent #33177

Closed
wants to merge 10 commits into from
Closed

http: destroy timeout socket by Agent #33177

wants to merge 10 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 30, 2020

Agent must destroy timeout socket when there is no any timeout
handler. Avoid socket hang on forever when the server don't send
any response back.

This is a try at landing #23752 which has become stale.

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

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. labels Apr 30, 2020
@ronag ronag force-pushed the default-timeout-handler branch 2 times, most recently from cbe03f2 to a55bb98 Compare April 30, 2020 21:06
@ronag ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 30, 2020
@ronag ronag mentioned this pull request Apr 30, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented May 2, 2020 •
edited
Loading

@nodejs/http @mcollina

P.S. @mcollina, shouldn't you be added to @nodejs/http as well?

@ronag ronag requested a review from lundibundi May 2, 2020 07:38
mcollina
mcollina approved these changes May 2, 2020
View reviewed changes
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

@ronag ronag requested review from jasnell and lpinca May 3, 2020 16:12
@ronag ronag force-pushed the default-timeout-handler branch from a55bb98 to b09ba53 Compare May 3, 2020 16:15
@nodejs-github-bot
Copy link
Collaborator

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

BridgeAR
BridgeAR approved these changes May 3, 2020
View reviewed changes
test/parallel/test-gc-http-client-timeout.js Outdated Show resolved Hide resolved
test/sequential/test-http-request-on-free-socket-timeout.js Outdated Show resolved Hide resolved
test/sequential/test-http-custom-timeout-handler.js Outdated Show resolved Hide resolved
test/sequential/test-http-custom-timeout-handler.js Outdated Show resolved Hide resolved
test/sequential/test-http-default-timeout-handler.js Outdated Show resolved Hide resolved
test/sequential/test-http-request-on-free-socket-timeout.js Outdated Show resolved Hide resolved
wa-Nadoo
wa-Nadoo reviewed May 3, 2020
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag force-pushed the default-timeout-handler branch from 7f1e4fd to 6084646 Compare May 4, 2020 04:33
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the wip Issues and PRs that are still a work in progress. label May 4, 2020
lundibundi
lundibundi approved these changes May 5, 2020
View reviewed changes
test/parallel/test-gc-http-client-timeout.js Outdated Show resolved Hide resolved
test/sequential/test-http-default-timeout-handler.js Outdated Show resolved Hide resolved
test/sequential/test-http-request-on-free-socket-timeout.js Outdated Show resolved Hide resolved
@ronag ronag marked this pull request as draft May 8, 2020 11:01
@ronag ronag removed the wip Issues and PRs that are still a work in progress. label May 8, 2020
omsmith and others added 5 commits May 10, 2020 22:18
@omsmith @ronag
http: set default timeout in agent keepSocketAlive
2e94eea 
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 <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@ronag @BridgeAR
Apply suggestions from code review
f27a327 
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
@ronag @wa-Nadoo
Update doc/api/http.md
30e5e6e 
Co-authored-by: Vitaly Kuzmich <wanadoo.by@gmail.com>
@ronag
fixup
71e011b
@ronag
fixup
6e9aa05
ronag and others added 5 commits May 10, 2020 22:18
@ronag ronag force-pushed the default-timeout-handler branch from f6939a0 to 56cae43 Compare May 10, 2020 20:18
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@lundibundi
Copy link
Member

ping @ronag I think this only needs a rebase and is ready otherwise? (it is still a draft)

@ronag ronag closed this Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wa-Nadoo wa-Nadoo wa-Nadoo left review comments

@lundibundi lundibundi lundibundi approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@lpinca lpinca Awaiting requested review from lpinca

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @lundibundi @mcollina @wa-Nadoo @BridgeAR @omsmith