net: rework autoSelectFamily implementation by ShogunPanda · Pull Request #46587 · nodejs/node · GitHub

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.

Review requested:

@nodejs/net

What is the performance impact of this pull request?

Putting on the tsc-agenda for visibility.

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?

cc @nodejs/cpp-reviewers

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?

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

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.

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

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?

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

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>

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

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.

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

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

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

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

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

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

Comment thread src/node_options.cc Outdated

anonrig reviewed Feb 9, 2023

View reviewed changes

cjihrig reviewed Feb 11, 2023

View reviewed changes

Comment thread doc/api/net.md Outdated

Comment thread doc/api/net.md Outdated

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

Comment thread src/tcp_wrap.cc Outdated

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

Comment thread test/parallel/test-http2-ping-settings-heapdump.js Outdated

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 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 and removed needs-ci PRs that need a full CI run. backported-to-v18.x labels Aug 14, 2023

mhdawson mentioned this pull request Aug 15, 2023

Backport 48969 to v18.x staging v2 #49183

Closed

mhdawson added the backport-open-v18.x 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 and removed backport-requested-v18.x labels Aug 17, 2023

ruyadorno mentioned this pull request Aug 17, 2023

v18.18.0 proposal #49220

Merged

	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 by ShogunPanda · Pull Request #46587 · nodejs/node · GitHub

Uh oh!

Uh oh!

net: rework autoSelectFamily implementation - #46587

net: rework autoSelectFamily implementation#46587nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom ShogunPanda:autoselectfamily-default-onShogunPanda/node:autoselectfamily-default-onCopy head branch name to clipboard

Conversation

ShogunPanda commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 9, 2023

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina commented Feb 11, 2023

Uh oh!

ShogunPanda commented Feb 11, 2023

Uh oh!

anonrig commented Feb 11, 2023

Uh oh!

ShogunPanda commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Feb 11, 2023

Uh oh!

ShogunPanda commented Feb 12, 2023

Uh oh!

mcollina commented Feb 12, 2023

Uh oh!

ShogunPanda commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tniessen commented Feb 17, 2023

Uh oh!

ShogunPanda commented Jul 27, 2023

Uh oh!

mhdawson commented Jul 27, 2023

Uh oh!

mhdawson commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juanarbol commented Jul 27, 2023

Uh oh!

juanarbol commented Jul 27, 2023

Uh oh!

ShogunPanda commented Jul 29, 2023

Uh oh!

mhdawson commented Aug 2, 2023

Uh oh!

mhdawson commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Aug 15, 2023

Uh oh!

Reviewers

net: rework autoSelectFamily implementation#46587
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ShogunPanda:autoselectfamily-default-on

ShogunPanda commented Feb 9, 2023 •
edited
Loading

ShogunPanda commented Feb 11, 2023 •
edited
Loading

ShogunPanda commented Feb 13, 2023 •
edited
Loading

mhdawson commented Jul 27, 2023 •
edited
Loading

mhdawson commented Aug 4, 2023 •
edited
Loading