dns: add promisified dns module

[cjihrig]

This is a work in progress attempt at a promisified DNS core module. I'm opening this early to see if people are generally interested in this approach. The entire API is not ported yet - mostly just missing lookup() and lookupService().

Some notes:

• The new stuff is added to require('dns').promises, as similarly done in the fs API. Like in fs, they are lazy loaded and output an experimental warning.
• There are currently no changes at the C++ layer because the DNS APIs come back to JavaScript to postprocess results before returning to user code.
• This currently relies on process.binding('util').createPromise and friends, which are currently facing potential removal in another PR.
• make test passed for me locally, so no regressions should exist in the callback based API.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/42/pipeline

[devsnek]

ideally we shouldn't be using those promise utils, they can't be optimised by v8

also should be looped into the new module naming thing as this can't be used first-class by esm

[cjihrig]

ideally we shouldn't be using those promise utils, they can't be optimised by v8

OK, got rid of those.

[addaleax]

LGTM once tests and documentation are added :)

[cjihrig]

The full API has been ported, and docs+tests added.

[addaleax]

Can we add a sibling of the Resolver class, with Resolver.prototype.promises as its prototype?

[cjihrig]

CI run: https://ci.nodejs.org/job/node-test-pull-request/15424/

Can we add a sibling of the Resolver class, with Resolver.prototype.promises as its prototype?

Do you mean like class PromiseResolver extends Resolver.prototype.promises (although that name is horrible :-))?

[addaleax]

@cjihrig Yes, basically – I’d call it Resolver too, and put in on dns.promises.Resolver. :)

[cjihrig]

@addaleax I added a dns.promises.Resolver class. I decided to get rid of Resolver.prototype.promises since there is a better way to access the functionality now.

[TimothyGu]

Right now we seem to have two implicit "subclasses" of GetNameInfoReqWrap: one having a .callback property and is used for dns, while the other has .resolve and .reject properties. I would rather make this distinction explicit by having a GetAddrInfoPromiseReqWrap subclass that extends GetAddrInfoReqWrap.

Same for the other wrapper classes.

[cjihrig]

@TimothyGu I was aiming to avoid a lot of unnecessary duplicate code. The wraps behave exactly the same in the C++ layer.

[TimothyGu]

@cjihrig Yeah I understand. What I'm trying to say is that you can have a JavaScript class that extends GetAddrInfoReqWrap. As long as its constructor calls super() properly it should work just fine. No duplication in C++ needed.

[joyeecheung]

@TimothyGu It still seems a bit complex if we hang all the data (host, port, bindingName, etc.) onto a req wrap with C++ binding even though the C++ binding may not necessarily read those properties from the req wrap. The only property that matters to C++ should be wrap.oncomplete, so it may be cleaner if we create two JS classes, one for the callback and the other one for promises, each with a .handle (or a symbol property) pointing to those req wraps and add the data properties to the JS instances of those classes. Then the main difference between those two classes would just be how we call resolve/reject/callback(null, result)/callback(dnsException(...)) in handle.oncomplete. (That's basically what the two types of wraps do in the fs promises implementation although those are implemented in C++)

(It's a bit unfortunate that the resolves have a ChannelWrap that's not req wrap as _handle though)

[joyeecheung]

On a second thought, it may not be feasible to separate the data properties to an outer object since we have been allowing users to retrieve them via the this bound to the callback...

EDIT: but we do not always bind the callback to the req wrap e.g. dns.lookup('127.0.0.1', function() { console.log(this) } gives you the global.

[cjihrig]

Green CI run: https://ci.nodejs.org/job/node-test-pull-request/15481/

[targos]

Should it say something like "the corresponding promises will be rejected with..."?

[cjihrig]

Good catch. Fixed.

[vsemozhetbyt]

Just some doc nits.

[vsemozhetbyt]

It seems this should be ### level, not ##, to reflect the hierarchy of not-promisified API.

[vsemozhetbyt]

It seems this should be #### level, not ###, to reflect the hierarchy of not-promisified API.

[vsemozhetbyt]

`Promises` -> `Promise`s?

[vsemozhetbyt]

a IPv4 addresses -> IPv4 addresses

[vsemozhetbyt]

a IPv6 addresses -> IPv6 addresses

[vsemozhetbyt]

Missing YAML.

[vsemozhetbyt]

[`dns. -> [`dnsPromises. in all table (with new bottom references)?

[vsemozhetbyt]

This note needs to be promisified)

[joyeecheung]

Code wise LGTM, regarding the internal structure we can refactor on top of this PR later.

[joyeecheung]

Can this be renamed to something like CallbackResolver or LegacyResolver to be more clear?

[vsemozhetbyt]

Should this be [`resolver.setServers()`][`dnsPromises.setServers()`]?

[vsemozhetbyt]

In all these hashes, _callback part needs to be deleted.

[benjamingr]

Maybe show an example with async/await since according to polls that's what most users are doing?

[benjamingr]

This defacto introduces promise cancellation to Node.js - can we add a note to this that the API for cancellation may change?

[benjamingr]

Alternatively, we can remove it and revisit.

[cjihrig]

cancel(), getServers(), and setServers() don't technically use promises (or callbacks for that matter). They are just synchronous methods provided on the Resolver class that are included here too for completeness.

[benjamingr]

@cjihrig this is a promise cancellation API though since you are rejecting all promises with an error "from a distance".

I'd really prefer it if we took extra caution around it since we'll want one API for cancellation and ideally users can use the same one for say... promisified timers, fs and DNS.

[joyeecheung]

Maybe we can avoid exposing this in the promise API, or rename it to something else?

[cjihrig]

I'm willing to remove it because it's a simple change. I don't agree with the change though, and I'm very -1 to renaming it.

[benjamingr]

Removing it until we have a more general strategy about cancellation with async functions SGTM. We can bring this up with TC39 at the next meeting if you'd like.

[benjamingr]

I would really prefer it if this method dind't do so much explicit construction:

async function createLookupPromise(family, hostname, all, hints, verbatim) {
  if (!hostname) {
    if (all) return [];
   else 
     return { address: null, family: family === 6 ? 6 : 4 };
  }
  const matchedFamily = isIP(hostname);
  if (matchedFamily !== 0) {
    const result = { address: hostname, family: matchedFamily }
    if(all) return [result];
    return result;
  }
  return await new Promise((resolve, reject) => {
    const req = new GetAddrInfoReqWrap();
    // ...
    req.resolve = resolve;
  }
}

[benjamingr]

Not a huge fan of how the code is written and I think it can be simpler - but it looks correct and the effort and progress is really nice.

Left some comment and LGTMd the current code as experimental to iterate on :)

[cjihrig]

Another CI run: https://ci.nodejs.org/job/node-test-pull-request/15536/ (AIX failures are unrelated, and I saw them on other CI runs today).

Landed in 7486c4d, with the cancel() removal in a77b30c (so it would make for an easy git revert).

[benjamingr]

Awesome, going to follow up with the nits from above :)