Make the promise.cancel() method actually work
#42
Conversation
|
Here is more info about the changes: sindresorhus/is-online#47 (comment) |
| @@ -83,16 +83,22 @@ const queryDns = (version, options) => { | |||
|
|
|||
| const socket = dns({ | |||
| retries: 0, | |||
| maxQueries: 1, | |||
There was a problem hiding this comment.
Another problem is the default maxQueries (10000) of dns-socket which is overkill because we just do one query at the same time
If we only do one query at the time, how does setting this change anything?
There was a problem hiding this comment.
dns-socket internally creates an empty array with 10000 empty values, later it may or not loop each entry searching for just one query and anyway is a waste of memory.
Check this line: https://github.com/mafintosh/dns-socket/blob/master/index.js#L31
| @@ -125,7 +131,7 @@ const queryDns = (version, options) => { | |||
| })(); | |||
|
|
|||
| promise.cancel = () => { | |||
| socket.cancel(); | |||
| socket.destroy(); | |||
There was a problem hiding this comment.
What's the difference between .cancel() and .destroy()?
There was a problem hiding this comment.
Well, socket.cancel() without argument does nothing because it needs the query id: https://github.com/mafintosh/dns-socket/blob/master/index.js#L217
In that case, doing "promise.cancel()" was basically no-op.
Now we are doing socket.destroy() and checking for socket.destroyed so we stop unnecessary queries.
sindresorhus
changed the title
promise.cancel() method actually work
This is needed for sindresorhus/is-online#47