Skip to content

net: fix address iteration with autoSelectFamily - #48258

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
indutny:fix/net-multi-connect
Jun 1, 2023
Merged

net: fix address iteration with autoSelectFamily#48258
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
indutny:fix/net-multi-connect

Conversation

@indutny

@indutnyindutny commented May 30, 2023

Copy link
Copy Markdown
Member

When autoSelectFamily is set to true, net.connect is supposed to try connecting to both IPv4 and IPv6, interleaving the address types. Instead, it appears that the array that holds the addresses in the order they should be attempted was never used after being populated.

@nodejs-github-bot

nodejs-github-bot commented May 30, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@indutny

indutny commented May 30, 2023

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/net

(I'd appreciate a backport to 18)

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels May 30, 2023
@juanarboljuanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@juanarbol

juanarbol commented May 31, 2023

Copy link
Copy Markdown
Member

I could try backporting to v18 when this lands.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot

nodejs-github-bot commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@ShogunPanda

ShogunPanda commented May 31, 2023

Copy link
Copy Markdown
Contributor

OMG!
Nice spot, thanks for finding it. You are absolutely right!

@ShogunPanda

ShogunPanda commented May 31, 2023

Copy link
Copy Markdown
Contributor

I'm gonna ask to fast track this, it might be also be used in #48189.

@ShogunPandaShogunPanda added the fast-track PRs that do not need to wait for 48 hours to land. label May 31, 2023
@github-actions

github-actionsBot commented May 31, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @ShogunPanda. Please 👍 to approve.

tniessen
tniessen approved these changes May 31, 2023
View reviewed changes

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

Woah, that, along with #48145, is a significant deviation from the RFC, and yet it's enabled by default. Is there any way to add a test for this to avoid this in the future?

@ShogunPanda

ShogunPanda commented May 31, 2023

Copy link
Copy Markdown
Contributor

@tniessen Yes, we can simulate bunch of addresses returned in order (like A, A, A, A, AAAA, AAAA, AAAA) and then we can verify we test them alternatively.

@indutny Can you please write such a test?

@indutny
net: fix address iteration with autoSelectFamily
71fe736
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
@indutny
indutnyforce-pushed the fix/net-multi-connect branch from eb3f503 to 71fe736CompareMay 31, 2023 15:03
@indutny

indutny commented May 31, 2023

Copy link
Copy Markdown
MemberAuthor

Good idea! @ShogunPanda would something like this suffice?

@ShogunPanda

ShogunPanda commented May 31, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Yes, that's exactly what I was thinking about, thanks!

@tniessen Are we good on merging this now?

@tniessen

tniessen commented May 31, 2023

Copy link
Copy Markdown
Member

@ShogunPanda Modulo approvals and CI, sure :)

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot

nodejs-github-bot commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

@juanarboljuanarbol added the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2023
@ShogunPandaShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 1, 2023
@ShogunPandaShogunPanda mentioned this pull request Jun 1, 2023
Merged
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 403a72f into nodejs:mainJun 1, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

Landed in 403a72f

This was referenced Jun 2, 2023
Open
Open
Open
targos pushed a commit that referenced this pull request Jun 4, 2023
@indutny@targos
net: fix address iteration with autoSelectFamily
950185b
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: #48258
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@targostargos mentioned this pull request Jun 4, 2023
Merged
This was referenced Jun 5, 2023
Open
Open
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jul 17, 2023
@indutny@juanarbol
net: fix address iteration with autoSelectFamily
f0d848a
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: nodejs#48258
Backport-PR-URL: nodejs#48275
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Jul 26, 2023
@indutny@mhdawson
net: fix address iteration with autoSelectFamily
4af24bc
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: nodejs#48258
Backport-PR-URL: nodejs#48275
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 4, 2023
@indutny@mhdawson
net: fix address iteration with autoSelectFamily
27a243f
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: nodejs#48258
Backport-PR-URL: nodejs#48275
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@mhdawsonmhdawson mentioned this pull request Aug 4, 2023
Closed
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@indutny@Ceres6
net: fix address iteration with autoSelectFamily
369b14c
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: nodejs#48258
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@indutny@Ceres6
net: fix address iteration with autoSelectFamily
52a7cab
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: nodejs#48258
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
@indutny@ruyadorno
net: fix address iteration with autoSelectFamily
d3637cd
When `autoSelectFamily` is set to `true`, `net.connect` is supposed to
try connecting to both IPv4 and IPv6, interleaving the address types.
Instead, it appears that the array that holds the addresses in the order
they should be attempted was never used after being populated.
PR-URL: #48258
Backport-PR-URL: #49016
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@ruyadornoruyadorno mentioned this pull request Aug 17, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPandaShogunPandaShogunPanda approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
@juanarboljuanarboljuanarbol approved these changes

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.needs-ciPRs that need a full CI run.netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@indutny@nodejs-github-bot@juanarbol@ShogunPanda@tniessen@lpinca@cjihrig@ruyadorno