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, dns: Socket should handle its output as input #44083

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

AdamMajer
Copy link
Contributor

As a consequence of #43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: #44003

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Aug 1, 2022
@AdamMajer
Copy link
Contributor Author

AdamMajer commented Aug 1, 2022

The problem seems to be here,

node/lib/net.js

Lines 1147 to 1152 in 7f7a899

if (!isWindows &&
dnsopts.family !== 4 &&
dnsopts.family !== 6 &&
dnsopts.hints === 0) {
dnsopts.hints = dns.ADDRCONFIG;
}

but changing this to recognize family as a string, fails a whole lot of tests and it wasn't clear to me if the correct way to fixing this is to change it in dns.js or in the connection function

As a consequence of nodejs#43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: nodejs#44003
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@nodejs-github-bot
Copy link
Collaborator

@kapouer
Copy link
Contributor

kapouer commented Aug 1, 2022

There is something strange happening:
./node test/parallel/test-net-socket-connect-without-cb.js -> OK
make test-ci-js -> NOT OK

EDIT: never mind. All is good.

@kvakil kvakil added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 1, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit ddd9c70 into nodejs:main Aug 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ddd9c70

danielleadams pushed a commit that referenced this pull request Aug 16, 2022
As a consequence of #43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: #44003
PR-URL: #44083
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
As a consequence of #43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: #44003
PR-URL: #44083
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
As a consequence of nodejs#43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: nodejs#44003
PR-URL: nodejs#44083
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 3, 2022
As a consequence of #43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: #44003
PR-URL: #44083
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol juanarbol mentioned this pull request Oct 4, 2022
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
As a consequence of #43014 ,
server sockets and others, once connected, report string family
names. But when feeding these to Socket.connect(), it passes
these to host resolution with a string for family while a numeric
family is expected internally. This results in wrong hints flags
to be set and resolution to fail.

As solution, is to add ability to handle both numeric and string
family names when doing lookup and connect.

Fixes: #44003
PR-URL: #44083
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol
Copy link
Member

This is causing CI failures in the v16.x release line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test: Error: getaddrinfo ENOTFOUND localhost
7 participants