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

doc: update family = 0 in dns.md #51483

Closed
wants to merge 2 commits into from
Closed

doc: update family = 0 in dns.md #51483

wants to merge 2 commits into from

Conversation

duncanchiu409
Copy link
Contributor

Fixes: #51482

First PR, please tell me if there is anything wrong. Thanks HEHE

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. labels Jan 16, 2024
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!

Copy link
Contributor

@domdomegg domdomegg left a comment

Choose a reason for hiding this comment

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

I think this is still not quite clear: this suggests only one will be returned. But on systems both are!

I also think we should update the dns promises and other relevant documentation to match, not just this one instance.

@duncanchiu409
Copy link
Contributor Author

duncanchiu409 commented Jan 17, 2024

The documentation said it won't return both, quoting this from https://man7.org/linux/man-pages/man3/getaddrinfo.3.html

The value AF_UNSPEC indicates that getaddrinfo() should return socket addresses for any address family (either IPv4 or IPv6, for example) that can be used with node and service.

I am not sure if there is any system exception.

@domdomegg
Copy link
Contributor

domdomegg commented Jan 17, 2024

On my system only one is returned. But some systems do seem to return multiple, e.g. whatever this is using under the hood:

https://www.jdoodle.com/execute-nodejs-online/

(try require('node:dns').lookup('localhost', { all: true, family: 0 }, console.log) - it outputs: null [ { address: '127.0.0.1', family: 4 }, { address: '::1', family: 6 } ])

I'm not sure which systems do act like this though.

@domdomegg
Copy link
Contributor

domdomegg commented Jan 17, 2024

Interestingly it looks like on all configurations I could see on GitHub actions both IPv4 and IPv6 addresses are returned:

https://github.com/domdomegg/nodejs-dns-lookup-family-testing/actions/runs/7549295297

This is strange, given running ubuntu 22.04 myself (which corresponds to ubuntu-latest in GitHub actions today) on Node 21 only returns IPv4. But my machine does support both given it can get IPv6 addresses when setting family = 6. Odd.

@ShogunPanda
Copy link
Contributor

ShogunPanda commented Jan 18, 2024

On my system only one is returned. But some systems do seem to return multiple, e.g. whatever this is using under the hood:

https://www.jdoodle.com/execute-nodejs-online/

(try require('node:dns').lookup('localhost', { all: true, family: 0 }, console.log) - it outputs: null [ { address: '127.0.0.1', family: 4 }, { address: '::1', family: 6 } ])

I'm not sure which systems do act like this though.

You are specifying all: true, so it will return all records received by the DNS, which means both IPv4 and IPv6. family: 0 behaves likes this when all: true is given.

When all: false (the default) is used, instead, only a single record will be returned, and if family: 0 is specified, it depends on the DNS server which family it belongs to.

Was I able to clarify it?

@duncanchiu409
Copy link
Contributor Author

I was expecting family: 0 and all: true would return one protocol only, either ip4 or ip6. But I think we could do a note for clearer explanation.

@duncanchiu409
Copy link
Contributor Author

I close this PR in-order to open a new PR for clarification.

@domdomegg
Copy link
Contributor

domdomegg commented Jan 19, 2024

On my system only one is returned. But some systems do seem to return multiple, e.g. whatever this is using under the hood:
https://www.jdoodle.com/execute-nodejs-online/
(try require('node:dns').lookup('localhost', { all: true, family: 0 }, console.log) - it outputs: null [ { address: '127.0.0.1', family: 4 }, { address: '::1', family: 6 } ])
I'm not sure which systems do act like this though.

You are specifying all: true, so it will return all records received by the DNS, which means both IPv4 and IPv6. family: 0 behaves likes this when all: true is given.

When all: false (the default) is used, instead, only a single record will be returned, and if family: 0 is specified, it depends on the DNS server which family it belongs to.

Was I able to clarify it?

I don't think this correct. On my system only one IP is returned, despite both being available:

require('node:dns').lookup('localhost', { all: true, family: 0 }, console.log)
// null [ { address: '127.0.0.1', family: 4 } ]

require('node:dns').lookup('localhost', { all: true, family: 4 }, console.log)
// null [ { address: '127.0.0.1', family: 4 } ]

require('node:dns').lookup('localhost', { all: true, family: 6 }, console.log)
// null [ { address: '::1', family: 6 } ]

It's that this is inconsistent between systems: some return one, some return both - and the docs don't reflect that (and it leads to this kind of confusion!).

@ShogunPanda
Copy link
Contributor

On my system (MacOS) both are returned when using all: true and family: 0. Of course this is system dependent for localhost (which might or might not bypass DNS resolution depending on the OS). What if you try with a remote host?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: dns docs for family = 0 and all = true is potentially misleading
4 participants