Skip to content
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

dns.promises.Resolver does not have a .cancel() method #33091

Closed
szmarczak opened this issue Apr 27, 2020 · 10 comments
Closed

dns.promises.Resolver does not have a .cancel() method #33091

szmarczak opened this issue Apr 27, 2020 · 10 comments
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@szmarczak
Copy link
Member

Is your feature request related to a problem? Please describe.

You cannot cancel pending DNS queries using dns.promises.Resolver. Note that dns.Resolver has a .cancel() method, but the async version does not.

Describe alternatives you've considered

There are no alternatives yet.

@rickyes
Copy link
Contributor

rickyes commented Apr 27, 2020

Yes, just mount the cancel function to the async Resolver.prototype. I'm working on this.

@rickyes
Copy link
Contributor

rickyes commented Apr 27, 2020

I'm not sure can add cancel in the promise API directly. Reference:21264#discussion_r196582158

/cc @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2020

It can be done, but people have opinions there. I'm personally OK with it, but you should probably check with others.

@jasnell
Copy link
Member

jasnell commented Apr 27, 2020

We cannot add a cancel to the Promises API directly but there are ways cancel can be accomplished...

The following is an example from a workshop I deliver to customers:

const EventEmitter = require('events');

// When using Race or All, if the underlying async API
// provides a mechanism for cancelation the pending
// operation, then a signaling mechanism like event
// emitter can be used to interject the ability to
// cancel. This, however, can be subject to race
// conditions that are not unlike thread synchronization

function createPromise(monitor, timeout, value) {
  return new Promise((res, rej) => {
    const t = setTimeout(() => {
      res(value);
      monitor.emit('resolved');
    }, timeout);
    monitor.on('resolved', () => {
      clearTimeout(t);
      rej('canceled')
    });
  });
}

function Foo(monitor) {
  return createPromise(monitor, 1000, 'B');
}

function Bar(monitor) {
  return createPromise(monitor, 10, 'A');
}

const monitor = new EventEmitter();

Promise.race([Foo(monitor), Bar(monitor)]).then(console.log);

Specifically, while Promises themselves cannot be canceled, if the underlying mechanism being used to schedule the asynchronous activity provides a means for canceling, then EventEmitter (or some other signaling mechanism) can be passed in to the Promise to trigger an early resolve or reject.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2020

If I'm remembering the linked discussion correctly, the issue was just about using the name "cancel" not actually implementing promise cancellation.

@devsnek devsnek added dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js. labels Apr 27, 2020
@szmarczak
Copy link
Member Author

Specifically, while Promises themselves cannot be canceled

From the docs:

Cancel all outstanding DNS queries made by this resolver. The corresponding callbacks will be called with an error with code ECANCELLED.

So I see no reason why can't we just reject(queryCancelledError).

@jasnell
Copy link
Member

jasnell commented Apr 27, 2020

When user code is using dnsPromises.Resolver directly that should be possible, with the caveat that it would have to cancel all outstanding Promises initiated by that Resolver in order to keep the API and implementation as simple as possible. For Promises returned by the default Resolver, there really wouldn't be a way to do it because the default Resolver is not exposed to user code via the API.

That is, this would work:

const { Resolver } = require('dns').promises;
const resolver = new Resolver();
resolver.setServers(['4.4.4.4']);

resolver.resolve4('example.org').then((addresses) => {});

resolver.cancel();

But canceling this would not:

const { resolve4 } = require('dns').promises;
resolve4('example.org').then((addresses) => {});

@szmarczak
Copy link
Member Author

szmarczak commented Apr 27, 2020

You can't cancel this either. What's your point?

const { resolve4 } = require('dns');
resolve4('example.org', (error, addresses) => {});

@szmarczak
Copy link
Member Author

If we need to be worried about cancelling these, it should be another issue...

@jasnell
Copy link
Member

jasnell commented Apr 27, 2020

You can't cancel this either. What's your point?

Be explicitly clear about expectations. I'm agreeing with you that implementing this should be possible. Just needs a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants