Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

doc: improve dns module's documentation #8747

Closed

Conversation

misterdjules
Copy link

Make the difference between dns.lookup and other functions even clearer.

Make the difference between dns.lookup and other functions even clearer.
@misterdjules misterdjules force-pushed the improve-dns-documentation branch from eb71830 to 5fbe669 Compare November 19, 2014 18:21
operating system.

It is implemented as a synchronous call to `getaddrinfo(3)` that runs on
libuv's threadpool. Because libuv's threadpool has a fixed size, it means that

Choose a reason for hiding this comment

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

It might be good to preface this sentence with something like "Though the call will be asynchronous from JavaScript's perspective, it is implemented internally [...]"

@chrisdickinson
Copy link

This LGTM.

@@ -51,6 +71,13 @@ Keep in mind that `err.code` will be set to `'ENOENT'` not only when
the domain does not exist but also when the lookup fails in other ways
such as no available file descriptors.

`dns.lookup` doesn't necessarily have anything to do with the DNS protocol.
It's only an operating system facility that can associate name with addresses,
and vice versa.
Copy link
Member

Choose a reason for hiding this comment

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

I would just say it's a wrapper around getaddrinfo, there is no need to hide it :-)

Copy link
Author

Choose a reason for hiding this comment

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

@saghul This information is present in the "Implementation considerations" section below, do you think it should be mentioned here instead?

Copy link
Member

Choose a reason for hiding this comment

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

Overall I think it should be clear from the start that dns.lookup == getaddrinfo, IMHO it would simplify the docs.

Choose a reason for hiding this comment

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

I agree. I knew there were two functions, one calling getaddrinfo in the thread pool, the other doing direct async dns via c-ares, and last time I used the dns module directly I had to spend a few minutes sifting through the documentation to figure out which of lookup() and resolve() was which... I kindof lament that the one that calls getaddrinfo isn't dns.getaddrinfo().

Copy link
Author

Choose a reason for hiding this comment

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

@saghul I agree with you, but my original concern was that by mentioning that dns.lookup == getaddrinfo in the introduction we would expose too much of the implementation's details to everyone right from the start. For instance, if we wanted to be precise for Windows users, we would need to mention GetAddrInfoW in addition to getaddrinfo (although they probably behave in a very similar way).

Instead, I thought that describing the general semantics and the concept of dns.lookup in the introduction would be sufficient, and would work for any implementation, even if these implementations change in the future. People interested in the implementation details could still consult the section that is reserved for that.

@sam-github The goal of this PR is exactly to address the problem you mentioned.

@sam-github
Copy link

I'm probably completely biased, as somebody who has implemented DNS protocols, so take this with a grain of salt... but I think a few words like the following is sufficient information for people who know something about addr resolution, and there is a risk that somebody who doesn't is unlikely to know anything about /etc/hosts or nsswitch.conf, and to be just puzzled by your additional explanation, even though it is quite accurate and detailed.

  • lookup calls the system resolver (via getaddrinfo) in the threadpool, so guarantees compatibility with other application's lookups, at the price of not being possible to parallelize more than the thread pool (link to uv threadpool docs... and wish there were node threadpool docs).
  • resolve uses a full async and parallel DNS resolver outside of the node threadpool, so is suitable for much higher parallelism, but can only find names resolvable directly via DNS.

@misterdjules
Copy link
Author

@sam-github @saghul Thank you very much for taking the time to review this proposal. I'm going to update it by taking your great suggestions into account and I'll let you know when I'm done.

@sam-github I like how you summed up the changes. I think it's definitely clearer and quicker to read for people who are familiar with node and name resolution, but in my opinion we need something a bit more detailed for people who are less familiar with these concepts.

I'd also like to see if we could use a more "collaborative" tool to work on this proposal. Maybe drafting this in the Wiki would be easier for collaboration?

misterdjules pushed a commit that referenced this pull request Dec 17, 2014
Make the difference between dns.lookup and other functions even clearer.

PR: #8747
Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
@tjfontaine
Copy link

This is definitely a step in the right direction, landed in 542234a, thanks!

@tjfontaine tjfontaine closed this Dec 17, 2014
mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
Make the difference between dns.lookup and other functions even clearer.

PR: nodejs#8747
Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
piscisaureus pushed a commit to piscisaureus/node2 that referenced this pull request Jan 10, 2015
Make the difference between dns.lookup and other functions even clearer.

PR-URL: nodejs/node-v0.x-archive#8747
Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>

Cherry-picked-from: nodejs/node-v0.x-archive@542234a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants