-
Notifications
You must be signed in to change notification settings - Fork 29k
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
[v18.x backport] net: fix address iteration with autoSelectFamily #48275
Conversation
Review requested:
|
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>
8132828
to
d8f15d5
Compare
Hey @indutny @ShogunPanda, I believe the introduced test for #48258 is covered by this, https://github.com/nodejs/node/pull/48275/files#diff-c97855fe951a3d9c4319418be09b97f6c2700565dbc65c47adfb3f520e8759e6R175 right? |
@juanarbol Nope, it's not. That test only included those addresses already in order, so there was no verification the attempt order was correct. In #48258 a test has been specifically included to verify the behavior. |
d8f15d5
to
4a90fbd
Compare
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>
4a90fbd
to
2d6a8c6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@ShogunPanda PTAL :-) |
This comment was marked as outdated.
This comment was marked as outdated.
This is hitting known flakes, see: #48300 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I don't think CI will be happy until the windows issue is solved; most jobs fail at this point. I prefer to wait for a bit more to kick a CI again. Or feel free to retry if needed. |
e4e363e
to
f1f6b38
Compare
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>
PR-URL: nodejs#45777 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#47860 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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>
f1f6b38
to
f0d848a
Compare
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>
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>
Thanks @juanarbol for the work here! @mhdawson helped get a few more commits in and it landed as part of the follow backport #49016 |
When
autoSelectFamily
is set totrue
,net.connect
is supposed totry 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: #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
This also includes the backport of #45777 which is required for this :)