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

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

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.

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.

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?

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?

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.

nit, freeSockets.shift()

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

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

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

Ping @nodejs/http

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.)

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.

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

@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.)

I would like to have a test for this behavior.

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.

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.

@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?

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

@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.

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

@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.

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

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.

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

lgtm

@rustyconover this has conflicts

Implemented in #33278

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 thread lib/_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 5af8b39 Compare 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 thread test/sequential/test-http-keepalive-lifo-sockets.js Outdated

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

Comment thread test/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 b43479f Compare 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

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

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

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

Uh oh!

Uh oh!

http: Change free sockets behavior to LIFO from FIFO. - #31526

http: Change free sockets behavior to LIFO from FIFO.#31526rustyconover wants to merge 2 commits intonodejs:masternodejs/node:masterfrom rustyconover:fix-https-agent-oldest-socketsrustyconover/node:fix-https-agent-oldest-socketsCopy head branch name to clipboard

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!

ronag Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

rustyconover Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

ronag Jan 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!

http: Change free sockets behavior to LIFO from FIFO.#31526
rustyconover wants to merge 2 commits into
nodejs:masterfrom
rustyconover:fix-https-agent-oldest-sockets

ronag commented Jan 27, 2020 •
edited
Loading

ronag commented Feb 28, 2020 •
edited
Loading

ronag commented Feb 28, 2020 •
edited
Loading