dns: refactor dns.resolve

[jasnell]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

dns

Description of change

This is in-progress and still needs additional work and review before it can land.

This seeks to address issue #1071 in a couple of ways.

1. An internal ref counter is used to count the number of outstanding
   dns.resolve() requests are still pending. If this number is > 0
   when dns.setServers() is called, an error will be thrown.
2. dns.resolve() will now return an EventEmitter that can emit three
   possible events: 'error', 'resolve' and 'complete'.

Previously, if there were any errors reported while setting up the
dns.resolve operation, they would be thrown immediately. However, any
errors that occur during the dns.operation would be passed to the
callback function as the first argument. Now, all errors are routed
through the 'error' event on the EventEmitter. This makes the error
handling more consistent but changes the flow since dns.resolve*()
will no longer throw immediately.

If a callback is passed to dns.resolve*(), which is the current
usage pattern, it is set as the handler for both the 'resolve'
and 'error' events.

Alternatively, it is now possible to omit the callback and add
listeners directly to the EventEmitter returned by dns.resolve_().
The 'resolve' listener is passed *only_ the results of the
resolve (errors are passed to 'error').

So, for example, you can continue to do this:

dns.resolve('www.example.org', 'A', (err, addresses) => {
  if (err) { /** ... **/ }
  // do stuff
});

Or you can do:

dns.resolve('www.example.org', 'A')
   .on('resolve', (addresses) => {
     // do stuff
   })
   .on('error', (error) => {
     // handle it
   })
   .on('complete', () => {
     // do this here because otherwise it'll throw.
     dns.setServers([]);
   }).

On the dns.setServers() part, the 'complete' event is emitted using
setImmediate(). This ensures that the event is not emitted until after
c-ares has an opportunity to clean up. This avoids the assert that brings
down the Node process if setServers is called at the wrong time.

/cc @mscdex @silverwind @nodejs/ctc

[silverwind]

Part 1 seems reasonable. I see that you seek to address the issue of the user calling setServers too early with part 2 and EventEmitters seem like the only way to communicate three states, but new API always comes at a cost.

Would it be unreasonable to ask the user to use setImmediate before setServers? Alternatively, how about adding a setImmediate to setServers when the refcounter is > 0? Would that reliably fix the crash?

[mscdex]

Would it be possible to just call automatically setServers() after being notified when the ref count reaches 0 and then call the callback passed to the particular dns resolution function after that?

[jasnell]

@silverwind ... Asking the user to use setImmediate with setServers() if they do so within the callback is reasonable. We could potentially modify setServers such that it recursively checks the refcounter when it's called and if it's > 0 it defers again, but then we could easily end up in a race condition where the setServers never actually get's called.

That is, this works:

dns.resolve('www.example.org', 'A', (err, addrs) => {
  setImmediate(() => dns.setServers([]));
});

But this doesn't:

'use strict';

const dns = require('dns');

dns.resolve('www.example.org', 'A', (err, addrs) => {
  console.log(addrs);
});
setImmediate(() => {
  dns.setServers([]);
});

While it is reasonable to ask the user to only call setServers() in the next event loop tick after the resolve completes, I think doing so is a bit problematic from a usability standpoint. For instance, that's already a viable solution for this and yet I still see users hit up against this problem.

@mscdex ... possibly but I think that would also put us into a race condition again, which I'm trying to avoid. I plan to keep looking at it tho as time allows. I'm definitely not saying that the approach taken in this PR is the "right* approach.

I will say that this revised version has a couple of things going for it:

1. It's easier to reason about errors. The existing API will either some errors immediately or pass them off to the callback. This will consistently trigger the on error handler (which can be handled by existing callbacks).
2. Existing calls should be largely unaffected unless that code is depending somehow on resolve throwing immediately. The existing API returns a QueryReqWrap object that is technically an undocumented internal API. This replaces it with a more supported public API that actually does something useful :-)

[jasnell]

Sigh... the original push missed a few edits that I forgot to git add before pushing... updated