http: destroy timeout socket by Agent #23752
Conversation
nodejs-github-bot
added
errors
fengmk2
force-pushed
the
default-timeout-handler
branch
from
e6b1b70 to
bb3eef5
Compare
|
@BridgeAR fixed follow your suggestions. |
fengmk2
force-pushed
the
default-timeout-handler
branch
2 times, most recently
from
89690c4 to
b3e5daa
Compare
| @@ -38,6 +39,11 @@ function getall() { | |||
| req.setTimeout(10, function() { | |||
| console.log('timeout (expected)'); | |||
| }); | |||
| req.on('error', (err) => { | |||
There was a problem hiding this comment.
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
|
@nodejs/http This could use some reviews. |
|
@nodejs/http bump.... 🔉 |
BridgeAR
added
the
semver-major
|
@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. |
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 | |||
There was a problem hiding this comment.
I think this should be ERR_HTTP_SOCKET_TIMEOUT, as this is triggered by http.
fengmk2
force-pushed
the
default-timeout-handler
branch
from
9e8265e to
52cb92d
Compare
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
| @@ -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 | |||
There was a problem hiding this comment.
@mcollina change to ERR_HTTP_SOCKET_TIMEOUT and add update on request.setTimeout().
| agent.removeSocket(s, options); | ||
| debug('CLIENT active socket destroy'); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think just doing:
s.destroy(new ERR_HTTP_SOCKET_TIMEOUT());should be enough now in master
|
@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? |
|
This seems to have been come stale. I'm closing it in favor of #33177 in order to try and land this change. |
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), orvcbuild test(Windows) passes