FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

net: add autoSelectFamily option by ShogunPanda · Pull Request #44731 · nodejs/node · GitHub

/ node Public

net: add autoSelectFamily option - #44731

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ShogunPanda:happy-eyeballs
Dec 3, 2022
ConversationCommits (2)ChecksFiles changed
Merged

net: add autoSelectFamily option#44731
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ShogunPanda:happy-eyeballs

Conversation

ShogunPanda commented Sep 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Sep 20, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

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
mcollina reviewed Sep 20, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please add a test for HTTPs too?

Comment thread doc/api/net.md Outdated
Comment thread deps/llhttp/CMakeLists.txt Outdated
Comment thread lib/net.js Outdated
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 commented Sep 20, 2022

Copy link
Copy Markdown
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.

aduh95 reviewed Sep 20, 2022
View reviewed changes
Comment thread lib/internal/errors.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated
Comment thread lib/net.js Outdated

Trott commented Sep 21, 2022

Copy link
Copy Markdown
Member

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

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

richardlau commented Sep 21, 2022

Copy link
Copy Markdown
Member

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

Details 
=== 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 commented Sep 21, 2022

Copy link
Copy Markdown
Contributor Author

Acknowledged!
I'm taking care of them locally.

mhdawson commented Sep 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 force-pushed the happy-eyeballs branch 2 times, most recently from 617860b to b1c7de5 Compare September 23, 2022 21:09
aduh95 reviewed Sep 24, 2022
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we add test for dnsPromises.Resolver as well?

Comment thread lib/_tls_wrap.js Outdated
Comment thread test/common/index.js Outdated
KhafraDev mentioned this pull request Sep 25, 2022
Merged
mhdawson mentioned this pull request Sep 26, 2022
Closed

Trott commented Sep 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Sep 26, 2022

Copy link
Copy Markdown
Member

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

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 commented Sep 27, 2022

Copy link
Copy Markdown
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.

mzz2017 mentioned this pull request Jun 19, 2023
Closed
federicobadini mentioned this pull request Jul 3, 2023
Merged
chinthakagodawita mentioned this pull request Jul 11, 2023
Merged
8 tasks
This was referenced Jul 11, 2023
Merged
Closed
oscrx mentioned this pull request Aug 4, 2023
Open
1 task
vidddd1 reviewed Sep 3, 2023
View reviewed changes
Comment thread doc/api/net.md

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

Sign in to view

Comment thread doc/api/net.md

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

Sign in to view

vidddd1 reviewed Sep 3, 2023
View reviewed changes

vidddd1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view
acmacalister mentioned this pull request Sep 19, 2023
Merged
saiarcot895 mentioned this pull request Oct 19, 2023
Closed
3 tasks
scotttrinh mentioned this pull request Nov 8, 2023
Merged
skyd0me mentioned this pull request Nov 14, 2023
Closed
This was referenced Nov 21, 2023
Closed
Closed
This was referenced Nov 24, 2023
Closed
Closed
This was referenced Dec 2, 2023
Closed
Closed
Closed
Closed
chinthakagodawita mentioned this pull request Jan 18, 2024
Merged
7 tasks
hainenber mentioned this pull request May 14, 2024
Closed
9 tasks
kt3k mentioned this pull request Oct 30, 2024
Closed
gauthier-th mentioned this pull request Jan 16, 2025
Merged
3 tasks
coderabbitai Bot mentioned this pull request Jul 18, 2026
Merged
This was referenced Aug 7, 2026
Merged
Merged
coderabbitai Bot mentioned this pull request Aug 18, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen left review comments
aduh95 aduh95 approved these changes
mcollina mcollina approved these changes
jasnell jasnell approved these changes
anonrig anonrig approved these changes
ronag ronag approved these changes
RafaelGSS RafaelGSS approved these changes
addaleax Awaiting requested review from addaleax
RaisinTen Awaiting requested review from RaisinTen
Trott Awaiting requested review from Trott
+3 more reviewers
apapirovski apapirovski left review comments
targos targos left review comments
vidddd1 vidddd1 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Happy Eyeballs support (address IPv6 issues in Node 17)
Back | FazBrowse Home | New Git URL