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

http: destroy timeout socket by Agent by fengmk2 · Pull Request #23752 · nodejs/node · GitHub

/ node Public

http: destroy timeout socket by Agent - #23752

Closed
fengmk2 wants to merge 1 commit into
nodejs:masterfrom
fengmk2:default-timeout-handler
ConversationCommits (1)ChecksFiles changed
Closed

http: destroy timeout socket by Agent#23752
fengmk2 wants to merge 1 commit into
nodejs:masterfrom
fengmk2:default-timeout-handler

Conversation

fengmk2 commented Oct 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

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 Oct 19, 2018
BridgeAR reviewed Oct 19, 2018
View reviewed changes
Comment thread test/sequential/test-http-default-timeout-handler.js Outdated
Comment thread test/sequential/test-http-default-timeout-handler.js Outdated
Comment thread test/sequential/test-http-default-timeout-handler.js Outdated
fengmk2 force-pushed the default-timeout-handler branch from e6b1b70 to bb3eef5 Compare October 20, 2018 13:00

fengmk2 commented Oct 20, 2018

Copy link
Copy Markdown
Contributor Author

@BridgeAR fixed follow your suggestions.

fengmk2 force-pushed the default-timeout-handler branch 2 times, most recently from 89690c4 to b3e5daa Compare October 20, 2018 13:29
fengmk2 commented Oct 20, 2018
View reviewed changes
Comment thread test/parallel/test-gc-http-client-timeout.js Outdated

fengmk2 Oct 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor 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

this is a break change, now timeout request will emit ERR_HTTP_SOCKET_TIMEOUT error if you don't handle the timeout event yourself.

fengmk2 force-pushed the default-timeout-handler branch from b3e5daa to 9e8265e Compare October 21, 2018 16:59

Trott commented Nov 20, 2018

Copy link
Copy Markdown
Member

@nodejs/http This could use some reviews.

antsmartian commented Dec 22, 2018

Copy link
Copy Markdown
Contributor

@nodejs/http bump.... 🔉

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 20, 2019

BridgeAR commented Dec 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@nodejs/http @nodejs/http2 @nodejs/streams @ronag this could use some reviews.

It should probably be reviewed in general idea wise before we ask for a rebase, since it took so long to look at this again.

mcollina requested changes Dec 20, 2019
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

This require a doc update, as our documentation for req.setTimeout states that

Emitted when the underlying socket times out from inactivity. This only notifies that the socket has been idle. The request must be aborted manually.

Comment thread doc/api/errors.md Outdated

mcollina Dec 20, 2019

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

I think this should be ERR_HTTP_SOCKET_TIMEOUT, as this is triggered by http.

ronag requested changes Dec 20, 2019
View reviewed changes
Comment thread lib/_http_agent.js Outdated
fengmk2 force-pushed the default-timeout-handler branch from 9e8265e to 52cb92d Compare December 21, 2019 03:54
http: destroy timeout socket by Agent
be95679 
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.
fengmk2 force-pushed the default-timeout-handler branch from 52cb92d to be95679 Compare December 21, 2019 03:58
fengmk2 commented Dec 21, 2019
View reviewed changes
Comment thread doc/api/errors.md
(reason phrase).

<a id="ERR_HTTP_SOCKET_TIMEOUT"></a>
### ERR_HTTP_SOCKET_TIMEOUT

fengmk2 Dec 21, 2019

Copy link
Copy Markdown
Contributor 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

@mcollina change to ERR_HTTP_SOCKET_TIMEOUT and add update on request.setTimeout().

ronag mentioned this pull request Jan 27, 2020
Closed
2 tasks
ronag reviewed Mar 11, 2020
View reviewed changes
Comment thread lib/_http_agent.js
agent.removeSocket(s, options);
debug('CLIENT active socket destroy');
}
}

ronag Mar 11, 2020

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

I think just doing:

s.destroy(new ERR_HTTP_SOCKET_TIMEOUT());

should be enough now in master

ronag commented Mar 11, 2020

Copy link
Copy Markdown
Member

@fengmk2 My opinion here as matured a bit given recent changes. I would be more positive to this PR. Would you still be interested in sorting out the conflicts and comments?

ronag mentioned this pull request Apr 30, 2020
Closed
4 tasks

ronag commented Apr 30, 2020

Copy link
Copy Markdown
Member

This seems to have been come stale. I'm closing it in favor of #33177 in order to try and land this change.

ronag closed this Apr 30, 2020
coderabbitai Bot mentioned this pull request Jun 25, 2026
Merged
2 tasks
coderabbitai Bot mentioned this pull request Aug 4, 2026
Merged
3 tasks
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

ronag ronag requested changes
BridgeAR BridgeAR left review comments
mcollina mcollina requested changes
jasnell jasnell approved these changes

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.

8 participants

Back | FazBrowse Home | New Git URL