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: enable autoSelectFamily by default by ShogunPanda · Pull Request #46790 · nodejs/node

Conversation

@ShogunPanda

@ShogunPanda ShogunPanda commented Feb 23, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
byF, ronag, treysis, and alexanderkasten reacted with hooray emoji
All reactions
  • 🎉 4 reactions
@nodejs-github-bot

nodejs-github-bot commented Feb 23, 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 23, 2023
@ShogunPanda ShogunPanda mentioned this pull request Feb 23, 2023
net: rework autoSelectFamily implementation #46587
Merged
@ShogunPanda ShogunPanda added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 23, 2023
mcollina
mcollina approved these changes Feb 23, 2023
View reviewed changes

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@richardlau richardlau mentioned this pull request Feb 23, 2023
"localhost" favours IPv6 in node v17, used to favour IPv4 #40537
Closed
@ShogunPanda
ShogunPanda force-pushed the autoselectfamily-default-on-effective branch from ffa07c3 to 07f04b2 Compare February 27, 2023 08:39
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Feb 27, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@ShogunPanda

ShogunPanda commented Feb 27, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Feb 27, 2023
View reviewed changes
Comment thread doc/api/cli.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.
Comment thread doc/api/net.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronag
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
jasnell approved these changes Mar 2, 2023
View reviewed changes
@ShogunPanda ShogunPanda added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Mar 3, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Mar 3, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 3, 2023

Copy link
Copy Markdown
Collaborator
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda ShogunPanda added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Mar 3, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Mar 4, 2023
CI Reliability 2023-03-04 nodejs/reliability#513
Open
17 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 5, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 5, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS
RafaelGSS approved these changes Apr 5, 2023
View reviewed changes

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95
aduh95 requested a review from Trott April 5, 2023 14:12
@Trott

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

image

I'll land this manually.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Apr 5, 2023
View reviewed changes
@ShogunPanda

ShogunPanda commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ShogunPanda @aduh95 @Trott
net: enable autoSelectFamily by default
8b51c1a 
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#46790
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@Trott
Trott force-pushed the autoselectfamily-default-on-effective branch from e76851c to 8b51c1a Compare April 5, 2023 15:32
@Trott
Trott merged commit 8b51c1a into nodejs:main Apr 5, 2023
@ShogunPanda

ShogunPanda commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

image

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

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

Landed in 8b51c1a
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

Thanks for all the hard work and patience on this!
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

ShogunPanda commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

ShogunPanda commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

ShogunPanda commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

Trott commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Apr 11, 2023
2023-04-18, Version 20.0.0 (Current) #47381
Merged
@cjihrig cjihrig mentioned this pull request Apr 14, 2023
test: update test-net-dns-lookup.js IPv6 address #47562
Closed
@danielbankhead danielbankhead mentioned this pull request May 2, 2023
feat: Retry Socket Connection Timeouts on Node 20+ googleapis/nodejs-storage#2187
Merged
@tniessen tniessen mentioned this pull request May 6, 2023
net: fix family autoselection timeout handling. #47860
Merged
@tniessen tniessen mentioned this pull request Jun 10, 2023
Revert "net: enable autoSelectFamily by default" #48403
Closed
@chinthakagodawita 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
7 of 8 tasks
@llllvvuu llllvvuu mentioned this pull request Jul 12, 2023
feat(anvil): add ability to listen on multiple IP addresses foundry-rs/foundry#5222
Merged
@csuermann 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 approved these changes
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@ronag ronag ronag approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes

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++. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

11 participants

@ShogunPanda @nodejs-github-bot @tniessen @thygasantos @Trott @RafaelGSS @debadree25 @mcollina @jasnell @ronag @aduh95
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.