net: enable autoSelectFamily by default by ShogunPanda · Pull Request #46790 · nodejs/node

ShogunPanda · 2023-02-23T09:58:50Z

This PR enables the family auto selection algorithm by default.

It also renames the --enable-network-family-autoselection in favor of --no-network-family-autoselection (the old one is still available as an alias).

nodejs-github-bot · 2023-02-23T09:58:54Z

Review requested:

@nodejs/net

mcollina

lgtm

ShogunPanda · 2023-02-27T08:50:26Z

@nodejs/tsc I need another TSC approval for this to land. Can you please help me?

doc/api/cli.md

doc/api/net.md

nodejs-github-bot · 2023-03-03T18:30:48Z

Commit Queue failed

- Loading data for nodejs/node/pull/46790
✔  Done loading data for nodejs/node/pull/46790
----------------------------------- PR info ------------------------------------
Title      net: enable autoSelectFamily by default (#46790)
Author     Paolo Insogna  (@ShogunPanda)
Branch     ShogunPanda:autoselectfamily-default-on-effective -> nodejs:main
Labels     c++, net, semver-major, needs-ci, commit-queue-squash
Commits    3
 - net: enable autoSelectFamily by default
 - net: linted code
 - doc: apply suggestions from PR review
Committers 2
 - Paolo Insogna 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/46790
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46790
Reviewed-By: Matteo Collina 
Reviewed-By: Robert Nagy 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: apply suggestions from PR review
   ℹ  This PR was created on Thu, 23 Feb 2023 09:58:50 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46790#pullrequestreview-1311021655
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/46790#pullrequestreview-1315324532
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/46790#pullrequestreview-1320904552
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-02-27T08:46:33Z: https://ci.nodejs.org/job/node-test-pull-request/50051/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - doc: apply suggestions from PR review
- Querying data for job/node-test-pull-request/50051/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4326246111

nodejs-github-bot · 2023-04-05T11:14:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50963/

nodejs-github-bot · 2023-04-05T12:10:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50964/

RafaelGSS

LGTM.

Trott · 2023-04-05T15:28:57Z

@nodejs/tsc @nodejs/net I chose the default connection timeout of 250ms by following the Happy Eyeballs spec. Do you think I should increase it (for instance, in the CI in order to get a green we need 2.5s)?

My inclination would be to split the tests up into multiple files and/or move them to sequential. (The test I flagged for this only failed if it was run with a lot of other tests--or itself--simultaneously. I couldn't get it to fail running in isolation.)

Trott · 2023-04-05T15:30:25Z

The last CI that ran was yellow, but there's still a (strange) failure showing up in the GitHub CI wideget:

I'll land this manually.

ShogunPanda · 2023-04-05T15:31:54Z

Sorry, I misexplained.
I was not referring to the CI this time. We can move those tests as sequential in the future as you suggested.
But do you think all these failures can reflect frequent problem for final users if we choose this timeout value?

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#46790 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

ShogunPanda · 2023-04-05T15:32:51Z

The last CI that ran was yellow, but there's still a (strange) failure showing up in the GitHub CI wideget:

I'll land this manually.

Looking at the logs, it seems a machine is out of disk space.

Anyway, I'll wait for you to land this. Thanks!

Trott · 2023-04-05T15:33:01Z

Landed in 8b51c1a

Trott · 2023-04-05T15:33:25Z

Thanks for all the hard work and patience on this!

Trott · 2023-04-05T15:33:59Z

Oh, gosh, it looks like maybe we didn't run CITGM.... 😬

ShogunPanda · 2023-04-05T15:34:15Z

My pleasure.❤️
I'll never give up after the CI! 😁

ShogunPanda · 2023-04-05T15:35:11Z

Oh, gosh, it looks like maybe we didn't run CITGM.... 😬

Lol.
Ok, let me know if there are failures and I'll address them immediately.

Trott · 2023-04-05T15:35:17Z

Oh, gosh, it looks like maybe we didn't run CITGM.... 😬

Not a big deal--we should definitely do it before landing, but we'll just do it now and if things blow up there, we'll know we have to be cautious about releasing it....

ShogunPanda · 2023-04-05T15:36:24Z

Sounds good. I'll try to promptly address any problem.

Trott · 2023-04-05T15:38:27Z

CITGM on the current main branch, now that this PR just landed: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3144/

For comparison, here's the most recent CITGM that wasn't for 16.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3141/ which was for #47182

Trott · 2023-04-05T21:17:42Z

Could definitely use a look from some more versed-in-CITGM folks (@nodejs/citgm, @nodejs/releasers) but I don't think there are any significant new problems with the CITGM results. The only one I noticed that seemed like maybe it's something to investigate is prom-client failing to install in about half of the runs. That didn't happen in the "control" run but I'm not really seeing how the issue is this change.

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3144/nodes=osx1015/testReport/junit/(root)/citgm/prom_client_v14_2_0/

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 23, 2023

ShogunPanda mentioned this pull request Feb 23, 2023

net: rework autoSelectFamily implementation #46587

Merged

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

mcollina approved these changes Feb 23, 2023

View reviewed changes

richardlau mentioned this pull request Feb 23, 2023

"localhost" favours IPv6 in node v17, used to favour IPv4 #40537

Closed

ShogunPanda force-pushed the autoselectfamily-default-on-effective branch from ffa07c3 to 07f04b2 Compare February 27, 2023 08:39

ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2023

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Feb 27, 2023

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

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

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

ronag approved these changes Feb 27, 2023

View reviewed changes

This was referenced Feb 28, 2023

CI Reliability 2023-02-28 nodejs/reliability#509

Open

CI Reliability 2023-03-01 nodejs/reliability#510

Open

CI Reliability 2023-03-02 nodejs/reliability#511

Open

jasnell approved these changes Mar 2, 2023

View reviewed changes

ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 3, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 3, 2023

ShogunPanda added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 3, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2023

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Mar 4, 2023

CI Reliability 2023-03-04 nodejs/reliability#513

Open

17 tasks

This comment was marked as outdated.

Sign in to view

RafaelGSS approved these changes Apr 5, 2023

View reviewed changes

aduh95 requested a review from Trott April 5, 2023 14:12

Trott approved these changes Apr 5, 2023

View reviewed changes

Trott force-pushed the autoselectfamily-default-on-effective branch from e76851c to 8b51c1a Compare April 5, 2023 15:32

Trott merged commit 8b51c1a into nodejs:main Apr 5, 2023

RafaelGSS mentioned this pull request Apr 11, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

cjihrig mentioned this pull request Apr 14, 2023

test: update test-net-dns-lookup.js IPv6 address #47562

Closed

danielbankhead mentioned this pull request May 2, 2023

feat: Retry Socket Connection Timeouts on Node 20+ googleapis/nodejs-storage#2187

Merged

tniessen mentioned this pull request May 6, 2023

net: fix family autoselection timeout handling. #47860

Merged

tniessen mentioned this pull request Jun 10, 2023

Revert "net: enable autoSelectFamily by default" #48403

Closed

chinthakagodawita mentioned this pull request Jul 11, 2023

[@types/node] update 'net.LookupFunction' to reflect that it looks up all addresses instead of one DefinitelyTyped/DefinitelyTyped#66031

Merged

8 tasks

llllvvuu mentioned this pull request Jul 12, 2023

feat(anvil): add ability to listen on multiple IP addresses foundry-rs/foundry#5222

Merged

csuermann mentioned this pull request Aug 5, 2024

Timeout of all vsh devices with Node-Red 4.0.2 csuermann/node-red-contrib-virtual-smart-home#261

Closed

dirkwa mentioned this pull request Jan 23, 2026

Fix AggregateError crashes on Node.js 20+ caused by Happy Eyeballs (autoSelectFamily) SignalK/signalk-node-red#61

Closed

Uh oh!

Conversation

ShogunPanda commented Feb 23, 2023

Uh oh!

nodejs-github-bot commented Feb 23, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

ShogunPanda commented Feb 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 3, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 5, 2023

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

ShogunPanda commented Apr 5, 2023

Uh oh!

ShogunPanda commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

ShogunPanda commented Apr 5, 2023

Uh oh!

ShogunPanda commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

ShogunPanda commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

Trott commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants