net: rework autoSelectFamily implementation #46587

ShogunPanda · 2023-02-09T15:28:59Z

This PR reworks the implementation of autoSelectFamily in net.connect to better swap internal TCPWrap when the connection is successful. It also fixes several issues with TLS.

It also add a global getter and setter setDefaultAutoSelectFamilyAttemptTimeout and getDefaultAutoSelectFamilyAttemptTimeout to allow global customization of the entire family autoselection algorithm.

Fixes #46669.
Fixes #46679.

nodejs-github-bot · 2023-02-09T15:29:03Z

Review requested:

@nodejs/net

src/node_options.cc

anonrig

What is the performance impact of this pull request?

doc/api/net.md

mcollina · 2023-02-11T19:39:57Z

Putting on the tsc-agenda for visibility.

ShogunPanda · 2023-02-11T19:42:07Z

I'll review comments on Mon.
For now I'm just anticipating that I needed to make a substantial change in the implementation to support this switch.

This will involve a C++ change that I tried but I'm not sure if it has side effects. How can I ping for help on that?

anonrig · 2023-02-11T19:42:51Z

cc @nodejs/cpp-reviewers

ShogunPanda · 2023-02-11T19:52:41Z

Thanks @anonrig!

~~I'll push my changes later tonight so they can start taking a look while I fix the last failing test.~~

Changes pushed.

@nodejs/cpp-reviewers @nodejs/net To simplify and stabilize the implementation I introduce a new "Reinitialize" method on the TCP wrap to create a new uv_tcp_t. This is better that then previous implementation I had that swapped TCPWrap after connecting since it has less side effects. But do you think the C++ is valid? Do you think it can have any side effect?

mcollina · 2023-02-11T22:52:44Z

@ShogunPanda I would split the C++ change from flipping the default into two different PRs, I think the C++ changes do not need to be semver-major.

ShogunPanda · 2023-02-12T00:03:24Z

You mean C++ changes first and then I flip the switch?
Consider that I discovered the flaw in the current implementation (which broke lot of tests) when enabling the feature.

mcollina · 2023-02-12T00:35:10Z

I think we should backport the fix, but not the switch of the default. Right now they would both be set as semver-major.

ShogunPanda · 2023-02-13T09:08:21Z

As @mcollina suggested I've reworked this PR to only fix the autoSelectFamily implementation.
This is no longer semver-major I guess and can be back ported.

@nodejs/cpp-reviewers @addaleax @jasnell Do you mind reviewing the C++ changes and tell me if there is anything wrong?

src/tcp_wrap.cc

addaleax · 2023-02-13T12:16:21Z

src/tcp_wrap.cc

+      &wrap, args.Holder(), args.GetReturnValue().Set(UV_EBADF));
+
+  Environment* env = wrap->env();
+  int r = uv_tcp_init(env->event_loop(), &wrap->handle_);


Should this have some sort of check like this?

Suggested change

int r = uv_tcp_init(env->event_loop(), &wrap->handle_);

CHECK(!IsAlive(this));

int r = uv_tcp_init(env->event_loop(), &wrap->handle_);

Yes, probably. I'll add this.

@addaleax I tried, and it always fails, both in TCPWrap::TCPWrap or in TCPWrap::Reinitialize. So I guess this is not needed. Or am I doing the wrong check?

Does that mean that the handle is not closed at this point? I don’t think it’s safe to call uv_..._init() functions on open handles? @bnoordhuis

The handle just received uv_tcp_init but never any open/connect methods. Does it still mean they are considered opened?

The pointer to the uv_tcp_t object needs to stay alive until uv_close() is done with it. At the very least you'd need to pass a callback that frees the memory afterwards.

That is fine. I can definetely do that.

Memory problems aside, do you foresee any problems in swapping the handler?

I think in theory this works, but it’s going to be a bit more complex than the code above.

For example, you’ll need to update all ConnectionWrap subclasses to have a pointer handle member rather than a plain member, and you’re going to have to use Environment::CloseHandle() rather than uv_close() so that the close callback gets tracked properly.

My gut feeling would be that at that point, it probably makes more sense to look into how to handle this more properly on the JS layer rather than trying to re-use TCPWrap instances.

Ok, you are probably right, I'm looking for too much troubles. I'll see how to fix it on the JS side.

@addaleax I finally solved (hopefully all) the issues without touching the C++ and swapping handles.
Thanks for not having me kill with my own hands :)

test/parallel/test-http2-ping-settings-heapdump.js

tniessen · 2023-02-17T14:20:03Z

The ASan failures are relevant, in particular, heap-use-after-free.

PR-URL: #46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ShogunPanda · 2023-07-27T14:18:07Z

@juanarbol After doublecheck with @mhdawson we agreed that this PR should indeed land on 18 as it is the foundation to fix the initially faulty network family selection feature.

mhdawson · 2023-07-27T14:21:31Z

I have a backported version of this. @juanarbol should I create a separate backport PR or would it makes sense to be part of #48275? The backport I have depends on 48275 landing first.

I also want to backport #47029 and
#48189 as landing 48275, 46587, 47029 and then 49189 allows 48796 to be landed on 18.x. 48796 has not landed yet but it fixes an issue I've been looking at.

mhdawson · 2023-07-27T14:28:41Z

@ShogunPanda mentioned that we might also want to include 48464 on the ones backported. I'll add that to my list above.

juanarbol · 2023-07-27T18:26:51Z

I have a backported version of this. @juanarbol should I create a separate backport PR or would it makes sense to be part of #48275? The backport I have depends on 48275 landing first.

You can push into my PR if you need to, I'm ok with whatever you prefer to do

juanarbol · 2023-07-27T18:31:38Z

@juanarbol That's wrong, they are unrelated.

When I tried to land this, I found that this was using at least one change made by #46790

ShogunPanda · 2023-07-29T08:40:43Z

Yeah, but I think @mhdawson resolved those conflicts, isn't it?

mhdawson · 2023-08-02T20:22:22Z

@juanarbol, @ShogunPanda correct I had a set of merged/tweaked PRs including this one. Thinking about it more I assume pushing to the existing PR does not makes sense I'll need additional backport PRs. Will start doing that.

PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson · 2023-08-04T15:44:10Z

@juanarbol, @ShogunPanda, @ruyadorno I submitted this PR - #49016 to backport this PR as part of the backport needed to land #48969. It includes the commits from #48275 so it can either be landed after that PR lands or the commits could be merged into that PR for landing. @ruyadorno hoping as the releaser for the nex 18.x release you can decide what is best.

PR-URL: #46587 Backport-PR-URL: #49016 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson · 2023-08-15T15:19:49Z

Backport to 18.x PR (second try) - #49183

PR-URL: nodejs#46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #46587 Backport-PR-URL: #49183 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ShogunPanda added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2023

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Feb 9, 2023

mcollina self-requested a review February 9, 2023 15:30

addaleax reviewed Feb 9, 2023

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

anonrig reviewed Feb 9, 2023

View reviewed changes

cjihrig reviewed Feb 11, 2023

View reviewed changes

doc/api/net.md Outdated Show resolved Hide resolved

doc/api/net.md Outdated Show resolved Hide resolved

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 11, 2023

ShogunPanda force-pushed the autoselectfamily-default-on branch from f4ffcb8 to c8b5f22 Compare February 13, 2023 09:04

ShogunPanda removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 13, 2023

ShogunPanda changed the title ~~net: enable autoSelectFamily by default~~ net: rework autoSelectFamily implementation Feb 13, 2023

RaisinTen reviewed Feb 13, 2023

View reviewed changes

src/tcp_wrap.cc Outdated Show resolved Hide resolved

addaleax reviewed Feb 13, 2023

View reviewed changes

mhdawson mentioned this pull request Feb 13, 2023

Node.js Technical Steering Committee (TSC) Meeting 2023-02-15 nodejs/TSC#1341

Closed

mcollina requested changes Feb 15, 2023

View reviewed changes

test/parallel/test-http2-ping-settings-heapdump.js Show resolved Hide resolved

addaleax mentioned this pull request Feb 15, 2023

Error [ERR_INTERNAL_ASSERTION] with autoSelectFamily enabled #46669

Closed

ShogunPanda force-pushed the autoselectfamily-default-on branch from 873bbcc to fe56de9 Compare February 21, 2023 10:41

ShogunPanda mentioned this pull request Feb 21, 2023

autoSelectFamily: true breaks TLS #46679

Closed

ShogunPanda force-pushed the autoselectfamily-default-on branch from fe56de9 to 8e3a06c Compare February 21, 2023 16:13

targos pushed a commit that referenced this pull request Mar 13, 2023

net: rework autoSelectFamily implementation

e0d098b

PR-URL: #46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

tniessen mentioned this pull request Apr 24, 2023

SIGNAL USERS READ THIS FIRST: code: 'ERR_INTERNAL_ASSERTION' in internalConnectMultiple #47644

Closed

ShogunPanda mentioned this pull request May 31, 2023

tls: reapply servername on happy eyeballs connect #48255

Merged

ShogunPanda removed the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Jul 27, 2023

mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 4, 2023

net: rework autoSelectFamily implementation

568975f

PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson mentioned this pull request Aug 4, 2023

[v18.x] Backport 48969 to v18.x staging #49016

Closed

ruyadorno pushed a commit that referenced this pull request Aug 14, 2023

net: rework autoSelectFamily implementation

73d930d

PR-URL: #46587 Backport-PR-URL: #49016 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. and removed needs-ci PRs that need a full CI run. backported-to-v18.x PRs backported to the v18.x-staging branch. labels Aug 14, 2023

mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023

net: rework autoSelectFamily implementation

5dc04c4

PR-URL: nodejs#46587 Backport-PR-URL: Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson mentioned this pull request Aug 15, 2023

Backport 48969 to v18.x staging v2 #49183

Closed

mhdawson added the backport-open-v18.x Indicate that the PR has an open backport. label Aug 15, 2023

mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023

net: rework autoSelectFamily implementation

6e19fdb

PR-URL: nodejs#46587 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno pushed a commit that referenced this pull request Aug 17, 2023

net: rework autoSelectFamily implementation

267439f

PR-URL: #46587 Backport-PR-URL: #49183 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. labels Aug 17, 2023

ruyadorno mentioned this pull request Aug 17, 2023

v18.18.0 proposal #49220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: rework autoSelectFamily implementation #46587

net: rework autoSelectFamily implementation #46587

ShogunPanda commented Feb 9, 2023 •

edited

nodejs-github-bot commented Feb 9, 2023

anonrig left a comment

mcollina commented Feb 11, 2023

ShogunPanda commented Feb 11, 2023

anonrig commented Feb 11, 2023

ShogunPanda commented Feb 11, 2023 •

edited

mcollina commented Feb 11, 2023

ShogunPanda commented Feb 12, 2023

mcollina commented Feb 12, 2023

ShogunPanda commented Feb 13, 2023 •

edited

addaleax Feb 13, 2023

ShogunPanda Feb 13, 2023

ShogunPanda Feb 13, 2023

addaleax Feb 13, 2023

ShogunPanda Feb 13, 2023

bnoordhuis Feb 17, 2023

ShogunPanda Feb 17, 2023

addaleax Feb 17, 2023

ShogunPanda Feb 17, 2023

ShogunPanda Feb 21, 2023

tniessen commented Feb 17, 2023

ShogunPanda commented Jul 27, 2023

mhdawson commented Jul 27, 2023

mhdawson commented Jul 27, 2023 •

edited

juanarbol commented Jul 27, 2023

juanarbol commented Jul 27, 2023

ShogunPanda commented Jul 29, 2023

mhdawson commented Aug 2, 2023

mhdawson commented Aug 4, 2023 •

edited

mhdawson commented Aug 15, 2023

	int r = uv_tcp_init(env->event_loop(), &wrap->handle_);
	CHECK(!IsAlive(this));
	int r = uv_tcp_init(env->event_loop(), &wrap->handle_);

net: rework autoSelectFamily implementation #46587

net: rework autoSelectFamily implementation #46587

Conversation

ShogunPanda commented Feb 9, 2023 • edited

nodejs-github-bot commented Feb 9, 2023

anonrig left a comment

Choose a reason for hiding this comment

mcollina commented Feb 11, 2023

ShogunPanda commented Feb 11, 2023

anonrig commented Feb 11, 2023

ShogunPanda commented Feb 11, 2023 • edited

mcollina commented Feb 11, 2023

ShogunPanda commented Feb 12, 2023

mcollina commented Feb 12, 2023

ShogunPanda commented Feb 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Feb 17, 2023

ShogunPanda commented Jul 27, 2023

mhdawson commented Jul 27, 2023

mhdawson commented Jul 27, 2023 • edited

juanarbol commented Jul 27, 2023

juanarbol commented Jul 27, 2023

ShogunPanda commented Jul 29, 2023

mhdawson commented Aug 2, 2023

mhdawson commented Aug 4, 2023 • edited

mhdawson commented Aug 15, 2023

ShogunPanda commented Feb 9, 2023 •

edited

ShogunPanda commented Feb 11, 2023 •

edited

ShogunPanda commented Feb 13, 2023 •

edited

mhdawson commented Jul 27, 2023 •

edited

mhdawson commented Aug 4, 2023 •

edited