http: fix socket re-use races by ronag · Pull Request #32000 · nodejs/node

ronag · 2020-02-28T10:55:22Z

This fixes two race conditions related to socket re-use in keep alive agents.

sockets in the free list, that has emitted 'timeout' should not be re-used
sockets that has been destroy():d but has not yet emitted 'close' should not be re-used

Refs: #31526 (comment)

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

ronag · 2020-02-28T10:59:42Z

I think this should be able to land as semver-minor bugfix. Though I'd like a second opinion on that.

ronag · 2020-02-28T11:01:13Z

freeLen used to include the socket that was removed, however I think that was a mistake as well and it's only used for debug logging.

mcollina

lgtm

nodejs-github-bot · 2020-02-28T11:23:01Z

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

mcollina · 2020-02-28T11:25:07Z

I think this should be able to land as semver-minor bugfix. Though I'd like a second opinion on that.

I would consider it a patch, it really fixes a bad bug.

nodejs-github-bot · 2020-02-28T16:00:07Z

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

lpinca · 2020-02-28T20:12:57Z

Is the socket destroyed when the 'timeout' event is emitted? If not why can't it be reused if it is still a working socket?

ronag · 2020-02-28T20:25:04Z

Is the socket destroyed when the 'timeout' event is emitted? If not why can't it be reused if it is still a working socket?

No, it's currently up to the user. Users often set a timeout and assume it is destroyed. The conservative/non-breaking solution is to assume the socket is not working if it emits timeout and simply not re-use it.

As a separate semver-major PR I would propose to actually destroy the socket on 'timeout'.

lpinca · 2020-02-28T20:41:52Z

If it is added back to the free socket list and only removed from the same list on timeout, who closes it? It is not clear to me.

ronag · 2020-02-28T20:45:05Z

If it is added back to the free socket list and only removed from the same list on timeout, who closes it? It is not clear to me.

You are absolutely right. I missed that. I will sort that out. Thank you.

ronag · 2020-02-28T21:29:20Z

Given #32000 (comment) I had to update this PR and it can no longer be two separate commits. Essentially, if a socket has a timeout while in the free list we destroy it, and then ensure that destroyed sockets are not re-used.

nodejs-github-bot · 2020-02-28T21:33:08Z

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

ronag · 2020-02-28T21:38:42Z

We only need to do this if re-using the socket. Move it to where it is put into the free list.

Whether and when a socket is destroyed or not after a timeout is up to the user. This leaves an edge case where a socket that has emitted 'timeout' might be re-used from the free pool. Even if destroy is called on the socket, it won't be removed from the freelist until 'close' which can happen several ticks later. Sockets are removed from the free list on the 'close' event. However, there is a delay between calling destroy() and 'close' being emitted. This means that it possible for a socket that has been destroyed to be re-used from the free list, causing unexpected failures.

lpinca · 2020-02-29T07:32:20Z

It seems good but we should update the documentation of the agent specifying that free sockets are destroyed when they time out.

ronag · 2020-02-29T08:37:50Z

@lpinca updated doc

ronag · 2020-03-02T09:41:08Z

@mcollina: There has been some changes here since you reviewed. You still approve?

nodejs-github-bot · 2020-03-02T09:41:40Z

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

nodejs-github-bot · 2020-03-07T17:21:36Z

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

ronag · 2020-03-07T19:52:01Z

Landed in 8700d89

Whether and when a socket is destroyed or not after a timeout is up to the user. This leaves an edge case where a socket that has emitted 'timeout' might be re-used from the free pool. Even if destroy is called on the socket, it won't be removed from the freelist until 'close' which can happen several ticks later. Sockets are removed from the free list on the 'close' event. However, there is a delay between calling destroy() and 'close' being emitted. This means that it possible for a socket that has been destroyed to be re-used from the free list, causing unexpected failures. PR-URL: #32000 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

fengmk2 · 2020-04-25T13:40:35Z

+ const sockets = agent.freeSockets;
+ for (const name of ObjectKeys(sockets)) {
+ if (sockets[name].includes(s)) {
+ return s.destroy();


should remove the destroy socket from freeSockets list immediately to prevent new requests from being sent through this socket.
if(sockets[name].includes(s)){s.destroy();returnagent.removeSocket(s,options);}

Refs: - nodejs/node#32000

mariodu · 2021-04-14T11:31:04Z

+ debug('CLIENT socket onTimeout');
+
+ // Destroy if in free list.
+ // TODO(ronag): Always destroy, even if not in free list.


Does this TODO plan to be completed? From the current usage, it may be a breaking change, which makes timeout change from idletimeout to datatimeout.

ronag added the http Issues or PRs related to the http subsystem. label Feb 28, 2020

ronag requested review from BridgeAR and mcollina February 28, 2020 10:55

ronag force-pushed the agent-freesocket-timeout branch from ee40778 to 3bd3270Compare February 28, 2020 10:56

ronag requested a review from jasnell February 28, 2020 10:59

ronag commented Feb 28, 2020
View reviewed changes

ronag force-pushed the agent-freesocket-timeout branch from 3bd3270 to d12a45eCompare February 28, 2020 11:13

ronag mentioned this pull request Feb 28, 2020
http: Change free sockets behavior to LIFO from FIFO. #31526
Closed
2 tasks

mcollina approved these changes Feb 28, 2020
View reviewed changes

ronag force-pushed the agent-freesocket-timeout branch from d12a45e to 1ce5152Compare February 28, 2020 11:21

ronag added semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 28, 2020

mcollina requested a review from lpinca February 28, 2020 11:25

mcollina added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 28, 2020

ronag removed the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 28, 2020

lundibundi approved these changes Feb 28, 2020
View reviewed changes

ronag commented Feb 28, 2020
View reviewed changes

Comment threadlib/_http_agent.js Outdated

ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 28, 2020

ronag force-pushed the agent-freesocket-timeout branch from 6017e92 to 542860fCompare February 28, 2020 21:28

ronag changed the title ~~http: keep alive socket re-use races~~http: fix socket re-use racesFeb 28, 2020

ronag commented Feb 28, 2020
View reviewed changes

ronag force-pushed the agent-freesocket-timeout branch from 8548113 to aaa4fd5Compare February 28, 2020 21:44

ronag force-pushed the agent-freesocket-timeout branch from aaa4fd5 to 09e3863Compare February 28, 2020 21:44

fixup: add doc note
ff470e1

ronag requested review from lundibundi and mcollina February 29, 2020 08:37

lpinca approved these changes Feb 29, 2020
View reviewed changes

lundibundi approved these changes Mar 2, 2020
View reviewed changes

Comment threadtest/parallel/test-http-agent-timeout.js
Comment threadtest/parallel/test-http-agent-timeout.js

fixup: nits
594e55d

ronag closed this Mar 7, 2020

MylesBorins mentioned this pull request Mar 10, 2020
v13.11.0 proposal #32185
Merged

targos mentioned this pull request Apr 22, 2020
v12.16.3 release proposal #33009
Merged

fengmk2 reviewed Apr 25, 2020
View reviewed changes

omsmith mentioned this pull request Apr 28, 2020
"max socket lifetime" Agent impacted by v13.11 socket timeout changes #33111
Closed

ronag mentioned this pull request May 22, 2020
TLSWrap objects grow with HTTP keep-alive enabled until out of memory error occurs #33468
Closed

killagu added a commit to node-modules/agentkeepalive that referenced this pull request Jun 14, 2020
fix: compatible with node v12.16.3
872206f
Refs: - nodejs/node#32000

killagu mentioned this pull request Jun 14, 2020
fix: compatible with node v12.16.3 node-modules/agentkeepalive#91
Merged
2 tasks

fengmk2 pushed a commit to node-modules/agentkeepalive that referenced this pull request Jun 15, 2020
fix: compatible with node v12.16.3 (#91)
4ba9f9c
Refs: - nodejs/node#32000

mariodu reviewed Apr 14, 2021
View reviewed changes

Uh oh!

Uh oh!

Conversation

ronag commented Feb 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Feb 28, 2020

Uh oh!

ronagFeb 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 28, 2020

Uh oh!

mcollina commented Feb 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 28, 2020

Uh oh!

lpinca commented Feb 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Feb 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Feb 28, 2020

Uh oh!

ronag commented Feb 28, 2020

Uh oh!

ronag commented Feb 28, 2020

Uh oh!

nodejs-github-bot commented Feb 28, 2020

Uh oh!

ronagFeb 28, 2020

Choose a reason for hiding this comment

Uh oh!

lpinca commented Feb 29, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Feb 29, 2020

Uh oh!

ronag commented Mar 2, 2020

Uh oh!

nodejs-github-bot commented Mar 2, 2020

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 7, 2020

Uh oh!

ronag commented Mar 7, 2020

Uh oh!

fengmk2Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

marioduApr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Feb 28, 2020•
edited
Loading

ronagFeb 28, 2020•
edited
Loading

lpinca commented Feb 28, 2020•
edited
Loading

ronag commented Feb 28, 2020•
edited
Loading

lpinca commented Feb 29, 2020•
edited
Loading