Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: add autoSelectFamily option #44731

Merged
merged 2 commits into from Dec 3, 2022

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented Sep 20, 2022

This PR loosely implements section 5 of RFC 8305 (Happy Eyeballs algorithm).

A new option autoSelectFamily is added to net.connect.
When set to a positive number (or true), the lookup phase will keep all records by setting all=true.
A connection attempt will be tried to all AAAA and A records (alternating families), in sequence, giving each connection autoSelectFamily milliseconds to be established.
Errors are raised only if no connection succeeded.

Fixes #41625.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Sep 20, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a test for HTTPs too?

doc/api/net.md Outdated Show resolved Hide resolved
deps/llhttp/CMakeLists.txt Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
@mcollina mcollina added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 20, 2022
@mcollina
Copy link
Member

@nodejs/tsc this is a notable change that we should land before v18 goes LTS. We have seen quite a few reports of bugs due to improper configuration of IPv6, making #41625 a necessity. For example, Docker on Mac only exposes ports on the IPv4 address, making all connections to localhost fail.

lib/internal/errors.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Sep 21, 2022

No comment (yet at least) one way or the other on the implementation, but +1000 to implementing happy eyeballs in Node.js.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 21, 2022
@richardlau
Copy link
Member

The two new tests fail for me (Linux x64) and also in GitHub actions.

=== release test-http-happy-eyeballs ===
Path: parallel/test-http-happy-eyeballs
Error: --- stderr ---
node:events:491
      throw er; // Unhandled 'error' event
      ^

Error: listen EADDRINUSE: address already in use :::33167
    at Server.setupListenHandle [as _listen2] (node:net:1649:16)
    at listenInCluster (node:net:1697:12)
    at doListen (node:net:1846:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:83:21)
Emitted 'error' event on Server instance at:
    at emitErrorNT (node:net:1676:8)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'EADDRINUSE',
  errno: -98,
  syscall: 'listen',
  address: '::',
  port: 33167
}

Node.js v19.0.0-pre
Command: out/Release/node /home/runner/work/node/node/test/parallel/test-http-happy-eyeballs.js
=== release test-net-happy-eyeballs ===
Path: parallel/test-net-happy-eyeballs
Error: --- stderr ---
node:events:491
      throw er; // Unhandled 'error' event
      ^

Error: listen EADDRINUSE: address already in use :::37301
    at Server.setupListenHandle [as _listen2] (node:net:1649:16)
    at listenInCluster (node:net:1697:12)
    at doListen (node:net:1846:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:83:21)
Emitted 'error' event on Server instance at:
    at emitErrorNT (node:net:1676:8)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'EADDRINUSE',
  errno: -98,
  syscall: 'listen',
  address: '::',
  port: 37301
}

Node.js v19.0.0-pre
Command: out/Release/node /home/runner/work/node/node/test/parallel/test-net-happy-eyeballs.js

===
=== 2 tests failed
===

@ShogunPanda
Copy link
Contributor Author

Acknowledged!
I'm taking care of them locally.

@mhdawson
Copy link
Member

mhdawson commented Sep 21, 2022

No comment (yet at least) one way or the other on the implementation, but +1000 to implementing happy eyeballs in Node.js.

Same goes for me as well.

@ShogunPanda ShogunPanda force-pushed the happy-eyeballs branch 2 times, most recently from 617860b to b1c7de5 Compare September 23, 2022 21:09
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can we add test for dnsPromises.Resolver as well?

lib/_tls_wrap.js Outdated Show resolved Hide resolved
test/common/index.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Sep 26, 2022

we should land before v18 goes LTS

@nodejs/tsc Two comments.

  1. If I understand correctly, this is a breaking change. It should be semver major. EDIT: Oh, wait, this is an option that is off by default? So people would need to opt in to get the fix? Oh, then I can't imagine this is controversial at all. Yes, let's get this working and landed! Woo hoo! 🎉 🚀
  2. However...I am in favor of landing this in the 18.x line as a special case of an important bug fix. Specifically:

Docker on Mac only exposes ports on the IPv4 address, making all connections to localhost fail.

@Trott
Copy link
Member

Trott commented Sep 26, 2022

I'm going to rebase and force push to at least get the tarball job working.

@Trott Trott added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 26, 2022
@mcollina
Copy link
Member

Oh, wait, this is an option that is off by default? So people would need to opt in to get the fix?

My case is that we should make it enabled by default. This causes so many headaches to newbie developers that know little of IPv4 vs IPv6 and why localhost is two IPs.

We should make this change before v18 goes LTS.

Krinkle added a commit to gruntjs/grunt-contrib-connect that referenced this pull request Jul 13, 2023
Upgrade grunt-contrib-internal from 6 to 8, which
updates CI from Node 10,12,14 to Node 14,16,18,20.

Fix default connect() hostname to work on Node 18+, where otherwise
CI fails as follows:

> Running "nodeunit:tests" (nodeunit) task
> Testing connect_test.jsFatal error: connect ECONNREFUSED ::1:8000
> Error: connect ECONNREFUSED ::1:8000
>  at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1494:16)

Previously, grunt-contrib-connect was passing empty string or null
down to `connect(,hostname)` to leave the default implicitly handled
by `connect` and Node's built-in `http.Server` class. Then, later,
it was doing a fallback to `0.0.0.0` but this only affected the CLI
output text, not the value passed to `connect()`.

Node 17+ changed per nodejs/node#40702 to
prefer IPv6 loopback when available (`::`) instead of IPv4 loopback
(`0.0.0.0`), unless `0.0.0.0` is explicitly passed.

This was fixed in Node 20, with `http.get` underlying `net` TCP module
implementing "Happy Eyeballs" (same as browsers and curl) which
basically means trying both IPv6 and IPv4. nodejs/node#44731

The workaround is to call `dns.setDefaultResultOrder('ipv4first')`
which ensures that the first try is over IPv4 instead of IPv6.
Krinkle added a commit to gruntjs/grunt-contrib-connect that referenced this pull request Jul 13, 2023
Upgrade grunt-contrib-internal from 6 to 8, which
updates CI from Node 10,12,14 to Node 14,16,18,20.

Fix default connect() hostname to work on Node 18+, where otherwise
CI fails as follows:

> Running "nodeunit:tests" (nodeunit) task
> Testing connect_test.jsFatal error: connect ECONNREFUSED ::1:8000
> Error: connect ECONNREFUSED ::1:8000
>  at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1494:16)

Previously, grunt-contrib-connect was passing empty string or null
down to `connect(,hostname)` to leave the default implicitly handled
by `connect` and Node's built-in `http.Server` class. Then, later,
it was doing a fallback to `0.0.0.0` but this only affected the CLI
output text, not the value passed to `connect()`.

Node 17+ changed per nodejs/node#40702 to
prefer IPv6 loopback when available (`::`) instead of IPv4 loopback
(`0.0.0.0`), unless `0.0.0.0` is explicitly passed.

This was fixed in Node 20, with `http.get` underlying `net` TCP module
implementing "Happy Eyeballs" (same as browsers and curl) which
basically means trying both IPv6 and IPv4. nodejs/node#44731

The workaround is to call `dns.setDefaultResultOrder('ipv4first')`
which ensures that the first try is over IPv4 instead of IPv6.
Krinkle added a commit to gruntjs/grunt-contrib-connect that referenced this pull request Jul 13, 2023
Upgrade grunt-contrib-internal from 6 to 8, which
updates CI from Node 10,12,14 to Node 14,16,18,20.

Fix default connect() hostname to work on Node 18+, where otherwise
CI fails as follows:

> Running "nodeunit:tests" (nodeunit) task
> Testing connect_test.jsFatal error: connect ECONNREFUSED ::1:8000
> Error: connect ECONNREFUSED ::1:8000
>  at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1494:16)

Previously, grunt-contrib-connect was passing empty string or null
down to `connect(,hostname)` to leave the default implicitly handled
by `connect` and Node's built-in `http.Server` class. Then, later,
it was doing a fallback to `0.0.0.0` but this only affected the CLI
output text, not the value passed to `connect()`.

Node 17+ changed per nodejs/node#40702 to
prefer IPv6 loopback when available (`::`) instead of IPv4 loopback
(`0.0.0.0`), unless `0.0.0.0` is explicitly passed.

This was fixed in Node 20, with `http.get` underlying `net` TCP module
implementing "Happy Eyeballs" (same as browsers and curl) which
basically means trying both IPv6 and IPv4. nodejs/node#44731

The workaround is to call `dns.setDefaultResultOrder('ipv4first')`
which ensures that the first try is over IPv4 instead of IPv6.
Krinkle added a commit to gruntjs/grunt-contrib-connect that referenced this pull request Jul 18, 2023
Upgrade grunt-contrib-internal from 6 to 8, which
updates CI from Node 10,12,14 to Node 14,16,18,20.

Fix default connect() hostname to work on Node 18+, where otherwise
CI fails as follows:

> Running "nodeunit:tests" (nodeunit) task
> Testing connect_test.jsFatal error: connect ECONNREFUSED ::1:8000
> Error: connect ECONNREFUSED ::1:8000
>  at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1494:16)

Previously, grunt-contrib-connect was passing empty string or null
down to `connect(,hostname)` to leave the default implicitly handled
by `connect` and Node's built-in `http.Server` class. Then, later,
it was doing a fallback to `0.0.0.0` but this only affected the CLI
output text, not the value passed to `connect()`.

Node 17+ changed per nodejs/node#40702 to
prefer IPv6 loopback when available (`::`) instead of IPv4 loopback
(`0.0.0.0`), unless `0.0.0.0` is explicitly passed.

This was fixed in Node 20, with `http.get` underlying `net` TCP module
implementing "Happy Eyeballs" (same as browsers and curl) which
basically means trying both IPv6 and IPv4. nodejs/node#44731

The workaround is to call `dns.setDefaultResultOrder('ipv4first')`
which ensures that the first try is over IPv4 instead of IPv6.
@@ -1630,6 +1647,7 @@ net.isIPv6('fhqwhgads'); // returns false

[IPC]: #ipc-support
[Identifying paths for IPC connections]: #identifying-paths-for-ipc-connections
[RFC 8305]: https://www.rfc-editor.org/rfc/rfc8305.txt

This comment was marked as spam.

@@ -1630,6 +1647,7 @@ net.isIPv6('fhqwhgads'); // returns false

[IPC]: #ipc-support
[Identifying paths for IPC connections]: #identifying-paths-for-ipc-connections
[RFC 8305]: https://www.rfc-editor.org/rfc/rfc8305.txt

This comment was marked as spam.

Copy link

@vidddd1 vidddd1 left a comment

Choose a reason for hiding this comment

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

@vidddd1

This comment was marked as spam.

@vidddd1

This comment was marked as spam.

@skyd0me skyd0me mentioned this pull request Nov 14, 2023
undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this pull request Mar 30, 2024
This commit upgrades Node.js version to v20.x in CI/CD environment.

Previously used Node 18.x is moving towards end-of-life, with a planned
date of 2025-04-30. In contrast, Node 20.x has been offering long-term
support (LTS) since 2023-10-24. This makes Node 20.x a stable and
recommended version for production environments.

This commit also configures `actions/setup-node` with the
`check-latest` flag to always use the latest Node 20.x version, keeping
CI/CD setup up-to-date with minimal maintenance.
Details:
- actions/setup-node#165
- actions/setup-node#160

Using Node 20.x in CI/CD environments provides better compatibility with
Electron v29.0 which moves to Node 20.x.
Details:
- electron/electron#40343

This upgrade improves network connection handling in CI/CD pipelines
(where issues occur due to GitHub runners not supporting IPv6).
Details:
- actions/runner#3138
- actions/runner-images#668
- actions/runner#3213
- actions/runner-images#9540

Node 20.x adopts the Happy Eyeballs algorithm for improved IPv6
connectivity.
- nodejs/node#40702
- nodejs/node#41625
- nodejs/node#44731

This mitigates issues like `UND_ERR_CONNECT_TIMEOUT` and localhost DNS
resolution in CI/CD environments:
Details:
- nodejs/node#40537
- actions/runner#3213
- actions/runner-images#9540

Node 20 introduces `setDefaultAutoSelectFamily`, a global function from
Node 19.4.0, enabling better IPv4 support, especially in environments
with limited or problematic IPv6 support.
Details:
- nodejs/node#45777

Node 20.x defaults to the new `autoSelectFamily`, improving network
connection reliability in GitHub runners lacking full IPv6 support.
Details:
- nodejs/node#46790
undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this pull request Mar 30, 2024
This commit upgrades Node.js version to v20.x in CI/CD environment.

Previously used Node 18.x is moving towards end-of-life, with a planned
date of 2025-04-30. In contrast, Node 20.x has been offering long-term
support (LTS) since 2023-10-24. This makes Node 20.x a stable and
recommended version for production environments.

This commit also configures `actions/setup-node` with the
`check-latest` flag to always use the latest Node 20.x version, keeping
CI/CD setup up-to-date with minimal maintenance.
Details:
- actions/setup-node#165
- actions/setup-node#160

Using Node 20.x in CI/CD environments provides better compatibility with
Electron v29.0 which moves to Node 20.x.
Details:
- electron/electron#40343

This upgrade improves network connection handling in CI/CD pipelines
(where issues occur due to GitHub runners not supporting IPv6).
Details:
- actions/runner#3138
- actions/runner-images#668
- actions/runner#3213
- actions/runner-images#9540

Node 20.x adopts the Happy Eyeballs algorithm for improved IPv6
connectivity.
- nodejs/node#40702
- nodejs/node#41625
- nodejs/node#44731

This mitigates issues like `UND_ERR_CONNECT_TIMEOUT` and localhost DNS
resolution in CI/CD environments:
Details:
- nodejs/node#40537
- actions/runner#3213
- actions/runner-images#9540

Node 20 introduces `setDefaultAutoSelectFamily`, a global function from
Node 19.4.0, enabling better IPv4 support, especially in environments
with limited or problematic IPv6 support.
Details:
- nodejs/node#45777

Node 20.x defaults to the new `autoSelectFamily`, improving network
connection reliability in GitHub runners lacking full IPv6 support.
Details:
- nodejs/node#46790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Happy Eyeballs support (address IPv6 issues in Node 17)