Skip to content

Commit

Permalink
lib: correct error.errno to always be numeric
Browse files Browse the repository at this point in the history
Historically `error.errno` of system errors thrown by Node.js
can sometimes be the same as `err.code`, which are string
representations of the error numbers. This is useless and incorrect,
and results in an information loss for users since then they
will have to resort to something like
`process.binding('uv'[`UV_${errno}`])` to get to the numeric
error codes.

This patch corrects this behavior by always setting `error.errno`
to be negative numbers. For fabricated errors like `ENOTFOUND`,
`error.errno` is now undefined since there is no numeric equivalent
for them anyway. For c-ares errors, `error.errno` is now undefined
because the numeric representations (negated) can be in conflict
with libuv error codes - this is fine since numeric codes was
not available for c-ares errors anyway.

Users can use the public API `util.getSystemErrorName(errno)`
to retrieve string codes for these numbers.

PR-URL: #28140
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
  • Loading branch information
joyeecheung committed Jun 17, 2019
1 parent 28d3f19 commit 1432065
Show file tree
Hide file tree
Showing 19 changed files with 98 additions and 65 deletions.
17 changes: 10 additions & 7 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ attempts to read a file that does not exist.
* `code` {string} The string error code
* `dest` {string} If present, the file path destination when reporting a file
system error
* `errno` {number|string} The system-provided error number
* `errno` {number} The system-provided error number
* `info` {Object} If present, extra details about the error condition
* `message` {string} A system-provided human-readable description of the error
* `path` {string} If present, the file path when reporting a file system error
Expand Down Expand Up @@ -448,13 +448,15 @@ system error.

### error.errno

* {string|number}
* {number}

The `error.errno` property is a negative number which corresponds
to the error code defined in [`libuv Error handling`].

On Windows the error number provided by the system will be normalized by libuv.

The `error.errno` property is a number or a string. If it is a number, it is a
negative value which corresponds to the error code defined in
[`libuv Error handling`]. See the libuv `errno.h` header file
(`deps/uv/include/uv/errno.h` in the Node.js source tree) for details. In case
of a string, it is the same as `error.code`.
To get the string representation of the error code, use
[`util.getSystemErrorName(error.errno)`].

### error.info

Expand Down Expand Up @@ -2365,6 +2367,7 @@ such as `process.stdout.on('data')`.
[`stream.write()`]: stream.html#stream_writable_write_chunk_encoding_callback
[`subprocess.kill()`]: child_process.html#child_process_subprocess_kill_signal
[`subprocess.send()`]: child_process.html#child_process_subprocess_send_message_sendhandle_options_callback
[`util.getSystemErrorName(error.errno)`]: util.html#util_util_getsystemerrorname_err
[`zlib`]: zlib.html
[ES Module]: esm.html
[ICU]: intl.html#intl_internationalization_support
Expand Down
6 changes: 1 addition & 5 deletions lib/internal/cluster/round_robin_handle.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const assert = require('internal/assert');
const net = require('net');
const { sendHelper } = require('internal/cluster/utils');
const uv = internalBinding('uv');
const { constants } = internalBinding('tcp_wrap');

module.exports = RoundRobinHandle;
Expand Down Expand Up @@ -58,10 +57,7 @@ RoundRobinHandle.prototype.add = function(worker, send) {
// Still busy binding.
this.server.once('listening', done);
this.server.once('error', (err) => {
// Hack: translate 'EADDRINUSE' error string back to numeric error code.
// It works but ideally we'd have some backchannel between the net and
// cluster modules for stuff like this.
send(uv[`UV_${err.errno}`], null);
send(err.errno, null);
});
};

Expand Down
21 changes: 13 additions & 8 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ function uvExceptionWithHostPort(err, syscall, address, port) {
const ex = new Error(`${message}${details}`);
Error.stackTraceLimit = tmpLimit;
ex.code = code;
ex.errno = code;
ex.errno = err;
ex.syscall = syscall;
ex.address = address;
if (port) {
Expand Down Expand Up @@ -455,8 +455,8 @@ function errnoException(err, syscall, original) {

// eslint-disable-next-line no-restricted-syntax
const ex = new Error(message);
// TODO(joyeecheung): errno is supposed to err, like in uvException
ex.code = ex.errno = code;
ex.errno = err;
ex.code = code;
ex.syscall = syscall;

// eslint-disable-next-line no-restricted-syntax
Expand Down Expand Up @@ -499,9 +499,9 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
Error.stackTraceLimit = 0;
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(`${syscall} ${code}${details}`);
// TODO(joyeecheung): errno is supposed to err, like in uvException
Error.stackTraceLimit = tmpLimit;
ex.code = ex.errno = code;
ex.errno = err;
ex.code = code;
ex.syscall = syscall;
ex.address = address;
if (port) {
Expand All @@ -520,9 +520,16 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
* @returns {Error}
*/
function dnsException(code, syscall, hostname) {
let errno;
// If `code` is of type number, it is a libuv error number, else it is a
// c-ares error code.
// TODO(joyeecheung): translate c-ares error codes into numeric ones and
// make them available in a property that's not error.errno (since they
// can be in conflict with libuv error codes). Also make sure
// util.getSystemErrorName() can understand them when an being informed that
// the number is a c-ares error code.
if (typeof code === 'number') {
errno = code;
// ENOTFOUND is not a proper POSIX error, but this error has been in place
// long enough that it's not practical to remove it.
if (code === lazyUv().UV_EAI_NODATA || code === lazyUv().UV_EAI_NONAME) {
Expand All @@ -539,10 +546,8 @@ function dnsException(code, syscall, hostname) {
Error.stackTraceLimit = 0;
// eslint-disable-next-line no-restricted-syntax
const ex = new Error(message);
// TODO(joyeecheung): errno is supposed to be a number / err, like in
Error.stackTraceLimit = tmpLimit;
// uvException.
ex.errno = code;
ex.errno = errno;
ex.code = code;
ex.syscall = syscall;
if (hostname) {
Expand Down
4 changes: 1 addition & 3 deletions lib/internal/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,7 @@ function makeSyncWrite(fd) {
writeBuffer(fd, chunk, 0, chunk.length, null, undefined, ctx);
if (ctx.errno !== undefined) {
const ex = errors.uvException(ctx);
// Legacy: net writes have .code === .errno, whereas writeBuffer gives the
// raw errno number in .errno.
ex.errno = ex.code;
ex.errno = ctx.errno;
return cb(ex);
}
cb();
Expand Down
60 changes: 40 additions & 20 deletions test/internet/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
const common = require('../common');
const { addresses } = require('../common/internet');
const { internalBinding } = require('internal/test/binding');
const { getSystemErrorName } = require('util');
const assert = require('assert');
const dns = require('dns');
const net = require('net');
Expand Down Expand Up @@ -71,7 +72,10 @@ function checkWrap(req) {
TEST(function test_reverse_bogus(done) {
dnsPromises.reverse('bogus ip')
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'EINVAL' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'EINVAL');
assert.strictEqual(getSystemErrorName(err.errno), 'EINVAL');
}));

assert.throws(() => {
dns.reverse('bogus ip', common.mustNotCall());
Expand Down Expand Up @@ -161,11 +165,13 @@ TEST(async function test_resolveMx(done) {
TEST(function test_resolveMx_failure(done) {
dnsPromises.resolveMx(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveMx(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -199,11 +205,13 @@ TEST(async function test_resolveNs(done) {
TEST(function test_resolveNs_failure(done) {
dnsPromises.resolveNs(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveNs(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -241,11 +249,13 @@ TEST(async function test_resolveSrv(done) {
TEST(function test_resolveSrv_failure(done) {
dnsPromises.resolveSrv(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveSrv(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -279,11 +289,13 @@ TEST(async function test_resolvePtr(done) {
TEST(function test_resolvePtr_failure(done) {
dnsPromises.resolvePtr(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolvePtr(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -322,11 +334,13 @@ TEST(async function test_resolveNaptr(done) {
TEST(function test_resolveNaptr_failure(done) {
dnsPromises.resolveNaptr(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveNaptr(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -369,11 +383,13 @@ TEST(async function test_resolveSoa(done) {
TEST(function test_resolveSoa_failure(done) {
dnsPromises.resolveSoa(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveSoa(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -407,11 +423,13 @@ TEST(async function test_resolveCname(done) {
TEST(function test_resolveCname_failure(done) {
dnsPromises.resolveCname(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveCname(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand Down Expand Up @@ -443,11 +461,13 @@ TEST(async function test_resolveTxt(done) {
TEST(function test_resolveTxt_failure(done) {
dnsPromises.resolveTxt(addresses.INVALID_HOST)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: 'ENOTFOUND' }));
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 'ENOTFOUND');
}));

const req = dns.resolveTxt(addresses.INVALID_HOST, function(err, result) {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, 'ENOTFOUND');

assert.strictEqual(result, undefined);

Expand All @@ -461,12 +481,12 @@ TEST(function test_resolveTxt_failure(done) {
TEST(function test_lookup_failure(done) {
dnsPromises.lookup(addresses.INVALID_HOST, 4)
.then(common.mustNotCall())
.catch(common.expectsError({ errno: dns.NOTFOUND }));
.catch(common.expectsError({ code: dns.NOTFOUND }));

const req = dns.lookup(addresses.INVALID_HOST, 4, (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.errno, dns.NOTFOUND);
assert.strictEqual(err.errno, 'ENOTFOUND');
assert.strictEqual(err.code, dns.NOTFOUND);
assert.strictEqual(err.code, 'ENOTFOUND');
assert.ok(!/ENOENT/.test(err.message));
assert.ok(err.message.includes(addresses.INVALID_HOST));

Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-child-process-execfilesync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ require('../common');
// works as expected.

const assert = require('assert');
const { getSystemErrorName } = require('util');
const { execFileSync } = require('child_process');
const msgOut = 'this is stdout';
const msgOutBuf = Buffer.from(`${msgOut}\n`);
Expand All @@ -20,7 +21,8 @@ const args = [
execFileSync(process.execPath, args, { maxBuffer: 1 });
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
assert.strictEqual(e.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(e.errno), 'ENOBUFS');
// We can have buffers larger than maxBuffer because underneath we alloc 64k
// that matches our read sizes.
assert.deepStrictEqual(e.stdout, msgOutBuf);
Expand All @@ -44,7 +46,8 @@ const args = [
);
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
assert.strictEqual(e.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(e.errno), 'ENOBUFS');
return true;
});
}
7 changes: 5 additions & 2 deletions test/parallel/test-child-process-execsync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ require('../common');
// works as expected.

const assert = require('assert');
const { getSystemErrorName } = require('util');
const { execSync } = require('child_process');
const msgOut = 'this is stdout';
const msgOutBuf = Buffer.from(`${msgOut}\n`);
Expand All @@ -20,7 +21,8 @@ const args = [
execSync(`"${process.execPath}" ${args.join(' ')}`, { maxBuffer: 1 });
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
assert.strictEqual(e.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(e.errno), 'ENOBUFS');
// We can have buffers larger than maxBuffer because underneath we alloc 64k
// that matches our read sizes.
assert.deepStrictEqual(e.stdout, msgOutBuf);
Expand All @@ -46,7 +48,8 @@ const args = [
);
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
assert.strictEqual(e.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(e.errno), 'ENOBUFS');
return true;
});
}
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-child-process-spawn-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

'use strict';
const common = require('../common');
const { getSystemErrorName } = require('util');
const spawn = require('child_process').spawn;
const assert = require('assert');
const fs = require('fs');
Expand All @@ -42,7 +43,7 @@ assert.strictEqual(enoentChild.stdio[2], enoentChild.stderr);

enoentChild.on('error', common.mustCall(function(err) {
assert.strictEqual(err.code, 'ENOENT');
assert.strictEqual(err.errno, 'ENOENT');
assert.strictEqual(getSystemErrorName(err.errno), 'ENOENT');
assert.strictEqual(err.syscall, `spawn ${enoentPath}`);
assert.strictEqual(err.path, enoentPath);
assert.deepStrictEqual(err.spawnargs, spawnargs);
Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-child-process-spawnsync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require('../common');

const assert = require('assert');
const spawnSync = require('child_process').spawnSync;
const { getSystemErrorName } = require('util');
const msgOut = 'this is stdout';
const msgOutBuf = Buffer.from(`${msgOut}\n`);

Expand All @@ -19,7 +20,8 @@ const args = [
const ret = spawnSync(process.execPath, args, { maxBuffer: 1 });

assert.ok(ret.error, 'maxBuffer should error');
assert.strictEqual(ret.error.errno, 'ENOBUFS');
assert.strictEqual(ret.error.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(ret.error.errno), 'ENOBUFS');
// We can have buffers larger than maxBuffer because underneath we alloc 64k
// that matches our read sizes.
assert.deepStrictEqual(ret.stdout, msgOutBuf);
Expand All @@ -39,7 +41,8 @@ const args = [
const ret = spawnSync(process.execPath, args);

assert.ok(ret.error, 'maxBuffer should error');
assert.strictEqual(ret.error.errno, 'ENOBUFS');
assert.strictEqual(ret.error.code, 'ENOBUFS');
assert.strictEqual(getSystemErrorName(ret.error.errno), 'ENOBUFS');
}

// Default maxBuffer size is 1024 * 1024.
Expand Down
Loading

0 comments on commit 1432065

Please sign in to comment.