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.reverse throws on error instead of invoking the callback #3112

Closed
andris9 opened this issue Sep 29, 2015 · 6 comments
Closed

dns.reverse throws on error instead of invoking the callback #3112

andris9 opened this issue Sep 29, 2015 · 6 comments
Labels
question Issues that look for answers.

Comments

@andris9
Copy link

andris9 commented Sep 29, 2015

node -v
v4.1.1

When I call dns.reverse with invalid ip argument (eg. false, undefined or an empty string) the method does not return the error with a callback like dns.resolve does but it throws instead. See the example below.

> require('dns').reverse('', function(err){
...     console.log('Should not throw');
... });
Error: getHostByAddr EINVAL
    at Object.exports._errnoException (util.js:837:11)
    at errnoException (dns.js:30:15)
    at Object.query [as reverse] (dns.js:233:20)
    at repl:1:16
    at REPLServer.defaultEval (repl.js:164:27)
    at bound (domain.js:250:14)
    at REPLServer.runBound [as eval] (domain.js:263:12)
    at REPLServer.<anonymous> (repl.js:393:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)

Expected output would be to see the string Should not throw but instead I see an error stack.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 29, 2015

This is expected. The general convention is to thow immediately on invalid input and pass the error to the callback if the error happens at some later point. This is not only for this function, but for a lot of other ones — invalid input should throw.

@ChALkeR ChALkeR added the question Issues that look for answers. label Sep 29, 2015
@ChALkeR ChALkeR closed this as completed Sep 29, 2015
@ChALkeR
Copy link
Member

ChALkeR commented Sep 29, 2015

See #2873 (comment).

@andris9
Copy link
Author

andris9 commented Sep 29, 2015

While I agree that passing non-string values to dns.reverse might throw an exception then doing the same for strings (even though invalidly formatted, eg. empty strings, invalid IP addresses etc.) seems kind of off.

For comparison dns.resolve does not do this, you might provide an empty string or invalid hostname but no exception is thrown, the error is returned via callback. I think that resolve and reverse should behave similarly or alternatively the docs should state the differences.

Here's some examples for current behavior:

// empty string
dns.resolve("", cb); // empty string, invokes callback with ENODATA
dns.reverse("", cb); // empty string, throws EINVAL
// invalidly formatted input
dns.resolve("\x00", cb); // zero byte, invalid hostname, invokes callback with ENOTFOUND
dns.resolve("\x01", cb); // invalid hostname, invokes callback with ENODATA
dns.reverse("1.2.3", cb); // invalid IPv4, throws EINVAL

@Slayer95
Copy link

Agreed that dns.reverse("1.2.3", cb); shouldn't be throwing errors.
That's not a logic error to be caught by throwing.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 28, 2015

@Slayer95 But that is a logic error — you are passing something that isn't syntactically a valid IP address as an argument that accepts only IP addresses.

evanlucas added a commit to evanlucas/node that referenced this issue Nov 25, 2015
Previously, if invalid input is passed to dns methods like dns.reverse,
an error would be thrown. This change instead passes the error to the
callback.

Fixes: nodejs#3882
Fixes: nodejs#3112
@titanism
Copy link

titanism commented Mar 2, 2023

Because of this and other issues, we built 🍊 Tangerine npm install tangerine which is a 1:1 drop-in replacement for the DNS module. The reverse method does not throw in Tangerine.

-const dns = require('dns');
+const Tangerine = require('tangerine');

-dns.reverse('1.2.3', cb); // would normally throw

+const tangerine = new Tangerine();

+const reverseCb = callbackify(tangerine.reverse);
+reverseCb('1.2.3', (err, result) => console.log(err, result)); // does not throw

// note that tangerine supports promises too!
// tangerine.reverse('1.2.3').then(console.log).catch(console.error);
// or
// await tangerine.reverse('1.2.3');

chriszarate added a commit to chriszarate/appServer that referenced this issue Jul 27, 2023
If an invalid IP address is passed to dns.reverse—for example, when receiving a IPv4-mapped IPv6 address like `::ffff:127.0.0.1`—it will throw instead of calling the callback with an error. See:

nodejs/node#3112
chriszarate added a commit to chriszarate/appServer that referenced this issue Jul 27, 2023
If an invalid IP address is passed to dns.reverse—for example, when receiving a IPv4-mapped IPv6 address like `::ffff:127.0.0.1`—it will throw instead of calling the callback with an error. See:

nodejs/node#3112
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants