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

http: destroy timeout socket by Agent by ronag · Pull Request #33177 · nodejs/node · GitHub

/ node Public

http: destroy timeout socket by Agent - #33177

Closed
ronag wants to merge 10 commits into
nodejs:masterfrom
nxtedition:default-timeout-handler
ConversationCommits (10)ChecksFiles changed
Closed

http: destroy timeout socket by Agent#33177
ronag wants to merge 10 commits into
nodejs:masterfrom
nxtedition:default-timeout-handler

Conversation

ronag commented Apr 30, 2020

Copy link
Copy Markdown
Member

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 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 force-pushed the default-timeout-handler branch 2 times, most recently from cbe03f2 to a55bb98 Compare April 30, 2020 21:06
ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 30, 2020
ronag mentioned this pull request Apr 30, 2020
Closed
4 tasks

nodejs-github-bot commented May 1, 2020

Copy link
Copy Markdown
Collaborator

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

ronag commented May 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/http @mcollina

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

ronag requested a review from lundibundi May 2, 2020 07:38
mcollina approved these changes May 2, 2020
View reviewed changes

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

ronag requested review from jasnell and lpinca May 3, 2020 16:12
ronag force-pushed the default-timeout-handler branch from a55bb98 to b09ba53 Compare May 3, 2020 16:15

nodejs-github-bot commented May 3, 2020

Copy link
Copy Markdown
Collaborator

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

BridgeAR approved these changes May 3, 2020
View reviewed changes
Comment thread test/parallel/test-gc-http-client-timeout.js Outdated
Comment thread test/sequential/test-http-request-on-free-socket-timeout.js Outdated
Comment thread test/sequential/test-http-custom-timeout-handler.js Outdated
Comment thread test/sequential/test-http-custom-timeout-handler.js Outdated
Comment thread test/sequential/test-http-default-timeout-handler.js Outdated
Comment thread test/sequential/test-http-request-on-free-socket-timeout.js Outdated
wa-Nadoo reviewed May 3, 2020
View reviewed changes
Comment thread doc/api/http.md Outdated

nodejs-github-bot commented May 3, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 3, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 3, 2020

Copy link
Copy Markdown
Collaborator

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

ronag force-pushed the default-timeout-handler branch from 7f1e4fd to 6084646 Compare May 4, 2020 04:33

nodejs-github-bot commented May 4, 2020

Copy link
Copy Markdown
Collaborator

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

ronag added the wip Issues and PRs that are still a work in progress. label May 4, 2020
lundibundi approved these changes May 5, 2020
View reviewed changes
Comment thread test/parallel/test-gc-http-client-timeout.js Outdated
Comment thread test/sequential/test-http-default-timeout-handler.js Outdated
Comment thread test/sequential/test-http-request-on-free-socket-timeout.js Outdated
ronag marked this pull request as draft May 8, 2020 11:01
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
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>
Apply suggestions from code review
f27a327 
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
Update doc/api/http.md
30e5e6e 
Co-authored-by: Vitaly Kuzmich <wanadoo.by@gmail.com>
fixup
71e011b
fixup
6e9aa05
ronag force-pushed the default-timeout-handler branch from f6939a0 to 56cae43 Compare May 10, 2020 20:18
BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

lundibundi commented Sep 2, 2020

Copy link
Copy Markdown
Member

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

ronag closed this Sep 22, 2020
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
BridgeAR BridgeAR approved these changes
jasnell Awaiting requested review from jasnell
lpinca Awaiting requested review from lpinca
+2 more reviewers
wa-Nadoo wa-Nadoo left review comments
lundibundi lundibundi approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL