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: clarify what dns.setResolvers() affects #25570

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@sam-github
Copy link
Member

sam-github commented Jan 18, 2019

It does not affect dns.lookup().

cf. #25560

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@sam-github sam-github requested a review from vsemozhetbyt Jan 18, 2019

@sam-github sam-github force-pushed the sam-github:doc-clarify-setservers branch from 5a91f8d to ccec12a Jan 18, 2019

The `dns.setServers()` method affects only the methods of [`dns.Resolver`][]
that have equivalents as `dns.*()` APIs (and specifically *not*
[`dns.lookup()`][]). It must not be called while those methods are in progress.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jan 18, 2019

Member

Nit: unneeded empty line?

Show resolved Hide resolved doc/api/dns.md Outdated
@lpinca

lpinca approved these changes Jan 18, 2019

Show resolved Hide resolved doc/api/dns.md Outdated

@sam-github sam-github force-pushed the sam-github:doc-clarify-setservers branch from ccec12a to 905a392 Jan 23, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 23, 2019

Also, calling dns.setServers() should always be okay in recent versions of Node. Calling resolver.setServers() is also okay, but will yield an error if there are active queries.

I haven't changed this text, and don't want to get into it here. I'll add it to my todo list. Can you reference the PR that did this? It'll need YAML change markers. If you don't know, I'll track it down some time.

The structure of the docs are confusing. The Resolver class refers to the global docs, but it really be the other way around, the Resolver methods should be documented, and the globals should state that there is a global Resolver. An astute reader could guess that its implemented like this, but it isn't clearly stated.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 23, 2019

I think the relevant PRs were #14891 (error out when active queries are on the resolver + change global default resolver when calling dns.setServers()) and, if you want, #14518 (adding the Resolver class).

doc: clarify what dns.setResolvers() affects
It does not affect dns.lookup().

@sam-github sam-github force-pushed the sam-github:doc-clarify-setservers branch from 905a392 to d003bcd Jan 24, 2019

@sam-github

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 25, 2019

Landed in 0ce615c

@sam-github sam-github closed this Jan 25, 2019

@sam-github sam-github deleted the sam-github:doc-clarify-setservers branch Jan 25, 2019

pull bot pushed a commit to zys-contribs/node that referenced this pull request Jan 25, 2019

doc: clarify what dns.setResolvers() affects
It does not affect dns.lookup().

PR-URL: nodejs#25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

addaleax added a commit that referenced this pull request Jan 28, 2019

doc: clarify what dns.setResolvers() affects
It does not affect dns.lookup().

PR-URL: #25570
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment