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 #23752

Closed
wants to merge 1 commit into from
Closed

http: destroy timeout socket by Agent #23752

wants to merge 1 commit into from

Conversation

fengmk2
Copy link
Contributor

@fengmk2 fengmk2 commented Oct 19, 2018 •
edited
Loading

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 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
@fengmk2
Copy link
Contributor Author

fengmk2 commented Oct 20, 2018

@BridgeAR fixed follow your suggestions.

@fengmk2 fengmk2 force-pushed the default-timeout-handler branch 2 times, most recently from 89690c4 to b3e5daa Compare October 20, 2018 13:29
fengmk2
fengmk2 commented Oct 20, 2018
View reviewed changes
test/parallel/test-gc-http-client-timeout.js
@@ -38,6 +39,11 @@ function getall() {
req.setTimeout(10, function() {
console.log('timeout (expected)');
});
req.on('error', (err) => {
Copy link
Contributor Author

@fengmk2 fengmk2 Oct 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Nov 20, 2018

@nodejs/http This could use some reviews.

@antsmartian
Copy link
Contributor

@nodejs/http bump.... 🔉

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

BridgeAR commented Dec 20, 2019 •
edited
Loading

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

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.

doc/api/errors.md Outdated
@@ -1545,6 +1545,11 @@ An attempt was made to operate on an already closed socket.

A call was made and the UDP subsystem was not running.

<a id="ERR_SOCKET_TIMEOUT"></a>
### ERR_SOCKET_TIMEOUT
Copy link
Member

Choose a reason for hiding this comment

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

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

ronag
ronag requested changes Dec 20, 2019
View reviewed changes
lib/_http_agent.js Show resolved Hide resolved
@fengmk2
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
fengmk2 commented Dec 21, 2019
View reviewed changes
doc/api/errors.md
@@ -1912,6 +1912,11 @@ removed: v10.0.0
Used when an invalid character is found in an HTTP response status message
(reason phrase).

<a id="ERR_HTTP_SOCKET_TIMEOUT"></a>
### ERR_HTTP_SOCKET_TIMEOUT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

I think just doing:

s.destroy(new ERR_HTTP_SOCKET_TIMEOUT());

should be enough now in master

@ronag
Copy link
Member

ronag commented Mar 11, 2020

@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 ronag mentioned this pull request Apr 30, 2020
Closed
4 tasks
@ronag
Copy link
Member

ronag commented Apr 30, 2020

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

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

@ronag ronag ronag requested changes

@BridgeAR BridgeAR BridgeAR left review comments

@mcollina mcollina mcollina requested changes

@jasnell 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
@fengmk2 @Trott @antsmartian @BridgeAR @ronag @mcollina @jasnell @nodejs-github-bot