dns.setServers doesn't play well with async code
#1071
Comments
|
Hm... this is not a bad suggestion at all, but I this is not the way c-ares works :) The thing that you propose will require creating and keeping alive separate channels in a hashmap. |
|
Ah, so there is a reason it's implemented in this way. Maybe something that should be kept in mind for #1013. |
|
Another solution would be to allow multiple "resolver" instances, e.g. support the following pattern: The default |
|
Please don't close this, I didn't mean that it wasn't feasible :) Just wanted to stress out how it will be implemented. |
This is what node-dns basically does. It's an option, but I think I'm leaning more towards the option object for simplicity. |
silverwind
changed the title
dns.setServers - Replace it with an option on resolvedns.setServers doesn't play well with async code
|
Atm, dns.setServers asserts when there are running dns queries or callbacks of dns queries. This crashes: var dns = require('dns');
var servers = dns.getServers();
dns.setServers(['208.67.222.222', '208.67.220.220']);
dns.resolve4('myip.opendns.com', function(err, addresses, family) {
dns.setServers(servers);
console.log('addresses:', addresses);
});This works: var dns = require('dns');
var servers = dns.getServers();
dns.setServers(['208.67.222.222', '208.67.220.220']);
dns.resolve4('myip.opendns.com', function(err, addresses, family) {
setTimeout(function() { dns.setServers(servers); }, 0);
console.log('addresses:', addresses);
}); |
|
I hope this idea hasn't died. I'd really like to see this functionality. The node-dns alternative doesn't appear to be functioning at all in 2.2.1. |
|
I have a change for this i mind after the new DNS implementation lands (#1843 (comment)) |
|
I'm going to add a |
|
It looks like #1843 might be stalling out (@mscdex). Thinking about this a bit, I'm wondering if a possible stop-gap "solution" would be to modify |
|
@jasnell I'm all for it if it avoids the crash. It would give the user a chance to workaround. Regarding alternatives, @mafintosh's dns-socket is a pretty flexible JS DNS solution I've been using recently. Maybe worth investigating how its perf compares to @mscdex's implementation. |
This seeks to address issue nodejs#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: ```js dns.resolve('www.example.org', 'A', (err, addresses) => { if (err) { /** ... **/ } // do stuff }); ``` Or you can do: ```js 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.
|
Hey @silverwind, does #13050 fix this? |
nodejs/node-v0.x-archive#9243 (comment) led me to realizing that the way
dns.setServersis implemented doesn't play too well with async code.The current target server is stored in a per-app global state, which would probably be fine with sync code, but complicates things if you want to query multiple servers with our async
resolve.I'd propose adding an options object to
resolvecontainingservers, while deprecatingdns.setServersin a major version. I think having an option object onresolvemethods could prove useful later.Note: I haven't checked if c-ares enables per-query target servers easily, maybe @indutny can comment on that part.
The text was updated successfully, but these errors were encountered: