http: Change free sockets behavior to LIFO from FIFO. by rustyconover · Pull Request #31526 · nodejs/node

rustyconover · 2020-01-26T19:24:59Z

Sockets are added to the free list with .push() but they were
being removed with .shift(). This meant the sockets where being
removed in FIFO order, but this changes it to LIFO. Since older
sockets may be closed based due to inactivity on the server it is
more likely that a socket that is recently used will be able to
successfully process the next request.

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows

ronag · 2020-01-27T09:34:44Z

On the other hand sockets that have not been used in a while might unnecessarily timeout. I think LIFO is fine but we do need to remove sockets from the free list when they timeout (which we currently don't).

I'm unsure whether LIFO or FIFO is better here...

Also, this might be a good idea to apply when the free list is full, i.e. instead of throwing away the most recent socket (as we currently do), we should throw away the least recently used.

rustyconover · 2020-01-27T15:04:19Z

Why would we want to remove then until we actually try them? That's the current behavior.

You have a good idea on the second part about what should be thrown away, updated commit pending.

ronag · 2020-01-27T15:06:04Z

Why would we want to remove then until we actually try them? That's the current behavior.

Because they have timed out. I don't think that's the current behavior?

rustyconover · 2020-01-27T15:13:32Z

Are there event handlers still reading from the socket once they are placed in the agent pool? I don't believe so. If there aren't any handlers waiting to read how will the code know the socket has timed out?

ronag · 2020-01-27T15:17:48Z

If there aren't any handlers waiting to read how will the code know the socket has timed out?

By adding a handler. See, #23752.

ronag · 2020-01-27T20:52:56Z

+ this.maxFreeSockets > 0 &&
+ count <= this.maxSockets) {
+ if (freeLen >= this.maxFreeSockets) {
+ const oldest = this.freeSockets[name].shift();


nit, freeSockets.shift()

ronag · 2020-01-27T20:53:09Z

+ if (freeLen >= this.maxFreeSockets) {
+ const oldest = this.freeSockets[name].shift();
+ oldest.destroy();
+ } else {


nit, else if (!freeSockets)

I'm not quite following, could you explain a bit more.

the block inside of the else is for the case when !freeSockets, could be simplified

it's a nit, no biggie

ronag · 2020-01-27T20:54:06Z

Would be nice if you could make a test for this.

jasnell · 2020-01-27T21:41:39Z

Defensively marking this semver-major for now. It's possible this wouldn't break anyone but we need to verify.

jasnell · 2020-01-27T21:42:07Z

Ping @nodejs/http

Trott · 2020-01-28T14:44:52Z

A benchmark would be good. We might already have a relevant benchmark. I'm not sure.

Would be nice if you could make a test for this.

At least in theory, isn't this an implementation detail the user need not worry about? (But I wouldn't oppose a test either. Just not sure if it's necessary.)

ronag · 2020-01-28T14:49:00Z

At least in theory, isn't this an implementation detail the user need not worry about?

Agreed. Nice to have.

Sockets are added to the free list with .push() but they were being removed with .shift(). This meant the sockets where being removed in FIFO order, but this changes it to LIFO. Since older sockets may be closed based due to inactivity on the server it is more likely that a socket that is recently used will be able to successfully process the next request. Rather than destroying the last used socket destroy the oldest socket in the free list in push() on the last recently used socket.

rustyconover · 2020-02-26T11:39:40Z

@ronag @Trott could you please mark this PR as author ready?

Trott · 2020-02-27T06:00:21Z

@ronag @Trott could you please mark this PR as author ready?

Although there's some ambiguity around the author ready label, I would say this is not yet author ready because it is semver-major and therefore requires two approvals from the TSC.

/ping @nodejs/tsc Please review! (@jasnell described marking this as semver-major as being done "defensively" so it might not be semver-major after all. Opinions one way or the other are welcome.)

mcollina

I would like to have a test for this behavior.

mcollina · 2020-02-27T08:43:50Z

Very good work. This is one of the main changes I did in https://github.com/mcollina/undici to improve the handling of keep-alive connections.

ronag · 2020-02-27T09:53:36Z

There is a related issue to this that we might want to consider before landing this.

We have a problem with sockets in the freelist that timeout are not removed from the list. This change might make that worse, since the least recently used socket is less likely to be used and thus timeout, then when it is actually used the request using it would fail.

rustyconover · 2020-02-28T04:07:02Z

@ronag Does this mean that the request isn't retried on another connection automatically or is the request simply failed with a connection reset error?

rustyconover · 2020-02-28T04:07:25Z

@ronag is there a test case that stresses this condition?

ronag · 2020-02-28T08:04:18Z

@ronag Does this mean that the request isn't retried on another connection automatically or is the request simply failed with a connection reset error?

If the user doesn't abort the request in a 'timeout' event handler (this is optional at the moment), the socket might be incorrectly re-used and the corresponding request would potentially fail, probably with ECONNRESET.

@ronag is there a test case that stresses this condition?

Nope, would be nice to have a tests for it.

ronag · 2020-02-28T09:50:04Z

Just to clarify, I believe my concern is already a problem, however this PR might make it worse.

ronag · 2020-02-28T11:17:42Z

@rustyconover: I openend a separate PR to address my concern. I think this PR is good as is once a test is added, though I would prefer to wait for #32000 to land before landing this.

rustyconover · 2020-02-29T05:08:00Z

@ronag I just wrote a test for the LIFO behavior of this PR. Let me know what you think.

rustyconover · 2020-02-29T05:08:58Z

I would like to have a test for this behavior.

@mcollina I just added a test.

Add a test that ensures the HTTP agent reuses sockets in a LIFO fashion rather than FIFO.

rustyconover · 2020-03-01T04:33:05Z

@mcollina I believe I've addressed your suggestions in the updated commits. Please let me know what you think. Thank you! 🙏

mcollina

lgtm

ronag · 2020-03-11T07:05:23Z

@rustyconover this has conflicts

mcollina · 2020-05-20T09:01:30Z

Implemented in #33278

rustyconover · 2020-05-20T14:00:36Z

Great! 👍

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 26, 2020

rustyconover requested a review from ronag January 27, 2020 16:28

ronag reviewed Jan 27, 2020
View reviewed changes

Comment threadlib/_http_agent.js Outdated

rustyconover requested a review from ronag January 27, 2020 18:06

ronag approved these changes Jan 27, 2020
View reviewed changes

ronag reviewed Jan 27, 2020
View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 27, 2020

rustyconover force-pushed the fix-https-agent-oldest-sockets branch from 4ef4a2d to 5af8b39Compare February 17, 2020 17:49

mcollina reviewed Feb 27, 2020
View reviewed changes

ronag mentioned this pull request Feb 28, 2020
http: fix socket re-use races #32000
Closed
4 tasks

rustyconover requested review from mcollina and ronag February 29, 2020 05:09

mcollina reviewed Feb 29, 2020
View reviewed changes

Comment threadtest/sequential/test-http-keepalive-lifo-sockets.js Outdated
Comment threadtest/sequential/test-http-keepalive-lifo-sockets.js Outdated
Comment threadtest/sequential/test-http-keepalive-lifo-sockets.js Outdated

test: Add test http agent keepalive LIFO behavior
b43479f
Add a test that ensures the HTTP agent reuses sockets in a LIFO fashion rather than FIFO.

rustyconover force-pushed the fix-https-agent-oldest-sockets branch from 9aad728 to b43479fCompare March 1, 2020 04:31

rustyconover requested a review from mcollina March 1, 2020 04:32

mcollina approved these changes Mar 1, 2020
View reviewed changes

delvedor mentioned this pull request May 7, 2020
http: added scheduling option to http agent #33278
Closed
4 tasks

mcollina closed this May 20, 2020

Uh oh!

Uh oh!

Conversation

rustyconover commented Jan 26, 2020

Uh oh!

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

Uh oh!

Uh oh!

rustyconover commented Jan 27, 2020

Uh oh!

ronag commented Jan 27, 2020

Uh oh!

rustyconover commented Jan 27, 2020

Uh oh!

ronag commented Jan 27, 2020

Uh oh!

Uh oh!

ronagJan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronagJan 27, 2020

Choose a reason for hiding this comment

Uh oh!

rustyconoverJan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronagJan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronagJan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronag commented Jan 27, 2020

Uh oh!

jasnell commented Jan 27, 2020

Uh oh!

jasnell commented Jan 27, 2020

Uh oh!

Trott commented Jan 28, 2020

Uh oh!

ronag commented Jan 28, 2020

Uh oh!

rustyconover commented Feb 26, 2020

Uh oh!

Trott commented Feb 27, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Feb 27, 2020

Uh oh!

ronag commented Feb 27, 2020

Uh oh!

rustyconover commented Feb 28, 2020

Uh oh!

rustyconover commented Feb 28, 2020

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!

ronag commented Feb 28, 2020

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!

rustyconover commented Feb 29, 2020

Uh oh!

rustyconover commented Feb 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustyconover commented Mar 1, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Mar 11, 2020

Uh oh!

mcollina commented May 20, 2020

ronag commented Jan 27, 2020•
edited
Loading

ronag commented Feb 28, 2020•
edited
Loading

ronag commented Feb 28, 2020•
edited
Loading