GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — net: rework autoSelectFamily implementation by ShogunPanda · Pull Request #46587 · nodejs/node

Conversation

@ShogunPanda

@ShogunPanda ShogunPanda commented Feb 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ShogunPanda 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

nodejs-github-bot commented Feb 9, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot 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
mcollina self-requested a review February 9, 2023 15:30
addaleax
addaleax reviewed Feb 9, 2023
View reviewed changes
Comment thread src/node_options.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Feb 9, 2023
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the performance impact of this pull request?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig reviewed Feb 11, 2023
View reviewed changes
Comment thread doc/api/net.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/net.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings. label Feb 11, 2023
@mcollina

mcollina commented Feb 11, 2023

Copy link
Copy Markdown
Member

Putting on the tsc-agenda for visibility.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda

ShogunPanda commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Feb 11, 2023

Copy link
Copy Markdown
Member

cc @nodejs/cpp-reviewers
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda

ShogunPanda commented Feb 11, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Feb 11, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda

ShogunPanda commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Feb 12, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda
ShogunPanda force-pushed the autoselectfamily-default-on branch from f4ffcb8 to c8b5f22 Compare February 13, 2023 09:04
@ShogunPanda

ShogunPanda commented Feb 13, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda ShogunPanda removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 13, 2023
@ShogunPanda ShogunPanda changed the title net: enable autoSelectFamily by default net: rework autoSelectFamily implementation Feb 13, 2023
RaisinTen
RaisinTen reviewed Feb 13, 2023
View reviewed changes
Comment thread src/tcp_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Feb 13, 2023
View reviewed changes
Comment thread src/tcp_wrap.cc Outdated

@addaleax addaleax Feb 13, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ShogunPanda ShogunPanda Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably. I'll add this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ShogunPanda ShogunPanda Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Feb 13, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ShogunPanda ShogunPanda Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Feb 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ShogunPanda ShogunPanda Feb 17, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine. I can definetely do that.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Feb 17, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@ShogunPanda ShogunPanda Feb 17, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ShogunPanda ShogunPanda Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson mhdawson mentioned this pull request Feb 13, 2023
Node.js Technical Steering Committee (TSC) Meeting 2023-02-15 nodejs/TSC#1341
Closed
mcollina
mcollina requested changes Feb 15, 2023
View reviewed changes
Comment thread test/parallel/test-http2-ping-settings-heapdump.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax mentioned this pull request Feb 15, 2023
Error [ERR_INTERNAL_ASSERTION] with autoSelectFamily enabled #46669
Closed
@tniessen

tniessen commented Feb 17, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda
ShogunPanda force-pushed the autoselectfamily-default-on branch from 873bbcc to fe56de9 Compare February 21, 2023 10:41
@ShogunPanda ShogunPanda mentioned this pull request Feb 21, 2023
autoSelectFamily: true breaks TLS #46679
Closed
@ShogunPanda
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
@ShogunPanda @targos
net: rework autoSelectFamily implementation
e0d098b 
PR-URL: #46587
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
Merged
@tniessen tniessen mentioned this pull request Apr 24, 2023
SIGNAL USERS READ THIS FIRST: code: 'ERR_INTERNAL_ASSERTION' in internalConnectMultiple #47644
Closed
@ShogunPanda ShogunPanda mentioned this pull request May 31, 2023
tls: reapply servername on happy eyeballs connect #48255
Merged
@ShogunPanda ShogunPanda removed the dont-land-on-v18.x label Jul 27, 2023
@ShogunPanda

ShogunPanda commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Jul 27, 2023

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Jul 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@juanarbol

juanarbol commented Jul 27, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@juanarbol

juanarbol commented Jul 27, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda

ShogunPanda commented Jul 29, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Aug 2, 2023

Copy link
Copy Markdown
Member

@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.
juanarbol reacted with thumbs up emoji juanarbol reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 4, 2023
@ShogunPanda @mhdawson
net: rework autoSelectFamily implementation
568975f 
PR-URL: nodejs#46587
Backport-PR-URL:
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson mhdawson mentioned this pull request Aug 4, 2023
[v18.x] Backport 48969 to v18.x staging #49016
Closed
@mhdawson

mhdawson commented Aug 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.
ruyadorno reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
@ShogunPanda @ruyadorno
net: rework autoSelectFamily implementation
73d930d 
PR-URL: #46587
Backport-PR-URL: #49016
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno 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 mhdawson mentioned this pull request Aug 15, 2023
Backport 48969 to v18.x staging v2 #49183
Closed
@mhdawson

mhdawson commented Aug 15, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson 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
@ShogunPanda @mhdawson
net: rework autoSelectFamily implementation
6e19fdb 
PR-URL: nodejs#46587
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 17, 2023
@ShogunPanda @ruyadorno
net: rework autoSelectFamily implementation
267439f 
PR-URL: #46587
Backport-PR-URL: #49183
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno added backported-to-v18.x and removed backport-requested-v18.x labels Aug 17, 2023
@ruyadorno ruyadorno mentioned this pull request Aug 17, 2023
v18.18.0 proposal #49220
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
@tniessen tniessen tniessen left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@RaisinTen RaisinTen Awaiting requested review from RaisinTen
@cjihrig cjihrig Awaiting requested review from cjihrig
@addaleax addaleax Awaiting requested review from addaleax
@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. net Issues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

autoSelectFamily: true breaks TLS Error [ERR_INTERNAL_ASSERTION] with autoSelectFamily enabled

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@ShogunPanda @nodejs-github-bot @mcollina @anonrig @tniessen @juanarbol @mhdawson @bnoordhuis @jasnell @addaleax @cjihrig @RaisinTen @ruyadorno
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.