Skip to content

dns: refactor dns.resolve

Rodrigo Muino Tomonari requested to merge github/fork/jasnell/dns-set-servers into master
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 https://github.com/nodejs/node/issues/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

Merge request reports