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

src, os, dgram, tty: migrate to internal errors #16567

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ checks or `abort()` calls in the C++ layer.

## System Errors

System errors are generated when exceptions occur within the program's
System errors are generated when exceptions occur within the Node.js
runtime environment. Typically, these are operational errors that occur
when an application violates an operating system constraint such as attempting
to read a file that does not exist or when the user does not have sufficient
Expand All @@ -471,7 +471,24 @@ of error codes and their meanings is available by running `man 2 intro` or
In Node.js, system errors are represented as augmented `Error` objects with
added properties.

### Class: System Error
### Class: SystemError

### error.info

`SystemError` instances may have an additional `info` property whose
Copy link
Member

@joyeecheung joyeecheung Oct 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, I think a more popular pattern in the userland is using the err.data property (correct me if I am wrong), I remember seeing some logging services/tools collect that automatically in addition to err.code...I remember there is another PR for crypto that use info. Is there a reason that we pick info instead of data?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen both: https://www.npmjs.com/package/verror uses *.info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note: would be easier to read if we have a syntax for documenting optional properties..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, definitely something worth working on

value is an object with additional details about the error conditions.

The following properties are provided:

* `code` {string} The string error code
* `errno` {number} The system-provided error number
* `message` {string} A system-provided human readable description of the error
* `syscall` {string} The name of the system call that triggered the error
* `path` {Buffer} When reporting a file system error, the `path` will identify
the file path.
* `dest` {Buffer} When reporting a file system error, the `dest` will identify
the file path destination (if any).


#### error.code

Expand Down Expand Up @@ -1379,6 +1396,13 @@ instance.setEncoding('utf8');
Used when an attempt is made to call [`stream.write()`][] after
`stream.end()` has been called.

<a id="ERR_SYSTEM_ERROR"></a>
### ERR_SYSTEM_ERROR

The `ERR_SYSTEM_ERROR` code is used when an unspecified or non-specific system
error has occurred within the Node.js process. The error object will have an
`err.info` object property with additional details.

<a id="ERR_TLS_CERT_ALTNAME_INVALID"></a>
### ERR_TLS_CERT_ALTNAME_INVALID

Expand Down
10 changes: 6 additions & 4 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,13 @@ function bufferSize(self, size, buffer) {
if (size >>> 0 !== size)
throw new errors.TypeError('ERR_SOCKET_BAD_BUFFER_SIZE');

try {
return self._handle.bufferSize(size, buffer);
} catch (e) {
throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e);
const ctx = {};
const ret = self._handle.bufferSize(size, buffer, ctx);
if (ret === undefined) {
throw new errors.Error('ERR_SOCKET_BUFFER_SIZE',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for avoiding changing the error type? Maybe a comment because this looks a bit odd at first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I really dislike how this was done, but I want to save it for a different PR to change it.

new errors.SystemError(ctx));
}
return ret;
}

Socket.prototype.bind = function(port_, address_ /*, callback*/) {
Expand Down
99 changes: 99 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@
// message may change, the code should not.

const kCode = Symbol('code');
const kInfo = Symbol('info');
const messages = new Map();

const { kMaxLength } = process.binding('buffer');
const { defineProperty } = Object;

// Lazily loaded
var util = null;
var buffer;

function makeNodeError(Base) {
return class NodeError extends Base {
Expand Down Expand Up @@ -58,6 +60,76 @@ function makeNodeError(Base) {
};
}

function lazyBuffer() {
if (buffer === undefined)
buffer = require('buffer').Buffer;
return buffer;
}

// A specialized Error that includes an additional info property with
// additional information about the error condition. The code key will
// be extracted from the context object or the ERR_SYSTEM_ERROR default
// will be used.
class SystemError extends makeNodeError(Error) {
constructor(context) {
context = context || {};
let code = 'ERR_SYSTEM_ERROR';
if (messages.has(context.code))
code = context.code;
super(code,
context.code,
context.syscall,
context.path,
context.dest,
context.message);
Object.defineProperty(this, kInfo, {
configurable: false,
enumerable: false,
value: context
});
}

get info() {
return this[kInfo];
}

get errno() {
return this[kInfo].errno;
}

set errno(val) {
this[kInfo].errno = val;
}

get syscall() {
return this[kInfo].syscall;
}

set syscall(val) {
this[kInfo].syscall = val;
}

get path() {
return this[kInfo].path !== undefined ?
this[kInfo].path.toString() : undefined;
}

set path(val) {
this[kInfo].path = val ?
lazyBuffer().from(val.toString()) : undefined;
}

get dest() {
return this[kInfo].path !== undefined ?
this[kInfo].dest.toString() : undefined;
}

set dest(val) {
this[kInfo].dest = val ?
lazyBuffer().from(val.toString()) : undefined;
}
}

class AssertionError extends Error {
constructor(options) {
if (typeof options !== 'object' || options === null) {
Expand Down Expand Up @@ -128,6 +200,7 @@ module.exports = exports = {
RangeError: makeNodeError(RangeError),
URIError: makeNodeError(URIError),
AssertionError,
SystemError,
E // This is exported only to facilitate testing.
};

Expand All @@ -144,6 +217,9 @@ module.exports = exports = {
// Any error code added here should also be added to the documentation
//
// Note: Please try to keep these in alphabetical order
//
// Note: Node.js specific errors must begin with the prefix ERR_

E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', '%s');
E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`);
Expand Down Expand Up @@ -334,6 +410,7 @@ E('ERR_STREAM_READ_NOT_IMPLEMENTED', '_read() is not implemented');
E('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event');
E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode');
E('ERR_STREAM_WRITE_AFTER_END', 'write after end');
E('ERR_SYSTEM_ERROR', sysError('A system error occurred'));
E('ERR_TLS_CERT_ALTNAME_INVALID',
'Hostname/IP does not match certificate\'s altnames: %s');
E('ERR_TLS_DH_PARAM_SIZE', (size) =>
Expand Down Expand Up @@ -371,6 +448,28 @@ E('ERR_VALUE_OUT_OF_RANGE', (start, end, value) => {
E('ERR_ZLIB_BINDING_CLOSED', 'zlib binding closed');
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed');

function sysError(defaultMessage) {
return function(code,
syscall,
path,
dest,
message = defaultMessage) {
if (code !== undefined)
message += `: ${code}`;
if (syscall !== undefined) {
if (code === undefined)
message += ':';
message += ` [${syscall}]`;
}
if (path !== undefined) {
message += `: ${path}`;
if (dest !== undefined)
message += ` => ${dest}`;
}
return message;
};
}

function invalidArgType(name, expected, actual) {
internalAssert(name, 'name is required');

Expand Down
34 changes: 28 additions & 6 deletions lib/os.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,43 @@ const { deprecate } = require('internal/util');
const { getCIDRSuffix } = require('internal/os');
const isWindows = process.platform === 'win32';

const errors = require('internal/errors');

const {
getCPUs,
getFreeMem,
getHomeDirectory,
getHostname,
getInterfaceAddresses,
getHomeDirectory: _getHomeDirectory,
getHostname: _getHostname,
getInterfaceAddresses: _getInterfaceAddresses,
getLoadAvg,
getOSRelease,
getOSType,
getOSRelease: _getOSRelease,
getOSType: _getOSType,
getTotalMem,
getUserInfo,
getUserInfo: _getUserInfo,
getUptime,
isBigEndian
} = process.binding('os');

function getCheckedFunction(fn) {
return function checkError(...args) {
const ctx = {};
const ret = fn(...args, ctx);
if (ret === undefined) {
const err = new errors.SystemError(ctx);
Error.captureStackTrace(err, checkError);
throw err;
}
return ret;
};
}

const getHomeDirectory = getCheckedFunction(_getHomeDirectory);
const getHostname = getCheckedFunction(_getHostname);
const getInterfaceAddresses = getCheckedFunction(_getInterfaceAddresses);
const getOSRelease = getCheckedFunction(_getOSRelease);
const getOSType = getCheckedFunction(_getOSType);
const getUserInfo = getCheckedFunction(_getUserInfo);

getFreeMem[Symbol.toPrimitive] = () => getFreeMem();
getHostname[Symbol.toPrimitive] = () => getHostname();
getHomeDirectory[Symbol.toPrimitive] = () => getHomeDirectory();
Expand Down
16 changes: 14 additions & 2 deletions lib/tty.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@ function ReadStream(fd, options) {
if (fd >> 0 !== fd || fd < 0)
throw new errors.RangeError('ERR_INVALID_FD', fd);

const ctx = {};
const tty = new TTY(fd, true, ctx);
if (ctx.code !== undefined) {
throw new errors.SystemError(ctx);
}

options = util._extend({
highWaterMark: 0,
readable: true,
writable: false,
handle: new TTY(fd, true)
handle: tty
}, options);

net.Socket.call(this, options);
Expand All @@ -69,8 +75,14 @@ function WriteStream(fd) {
if (fd >> 0 !== fd || fd < 0)
throw new errors.RangeError('ERR_INVALID_FD', fd);

const ctx = {};
const tty = new TTY(fd, false, ctx);
if (ctx.code !== undefined) {
throw new errors.SystemError(ctx);
}

net.Socket.call(this, {
handle: new TTY(fd, false),
handle: tty,
readable: false,
writable: true
});
Expand Down
78 changes: 78 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "node_internals.h"
#include "async-wrap.h"
#include "v8-profiler.h"
#include "node_buffer.h"

#if defined(_MSC_VER)
#define getpid GetCurrentProcessId
Expand Down Expand Up @@ -228,4 +229,81 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type,
}
}

void CollectExceptionInfo(Environment* env,
v8::Local<v8::Object> obj,
int errorno,
const char* err_string,
const char* syscall,
const char* message,
const char* path,
const char* dest) {
obj->Set(env->errno_string(), v8::Integer::New(env->isolate(), errorno));

obj->Set(env->context(), env->code_string(),
OneByteString(env->isolate(), err_string)).FromJust();

if (message != nullptr) {
obj->Set(env->context(), env->message_string(),
OneByteString(env->isolate(), message)).FromJust();
}

v8::Local<v8::Value> path_buffer;
if (path != nullptr) {
path_buffer =
Buffer::Copy(env->isolate(), path, strlen(path)).ToLocalChecked();
obj->Set(env->context(), env->path_string(), path_buffer).FromJust();
}

v8::Local<v8::Value> dest_buffer;
if (dest != nullptr) {
dest_buffer =
Buffer::Copy(env->isolate(), dest, strlen(dest)).ToLocalChecked();
obj->Set(env->context(), env->dest_string(), dest_buffer).FromJust();
}

if (syscall != nullptr) {
obj->Set(env->context(), env->syscall_string(),
OneByteString(env->isolate(), syscall));
}
}

void Environment::CollectExceptionInfo(v8::Local<v8::Value> object,
int errorno,
const char* syscall,
const char* message,
const char* path) {
if (!object->IsObject() || errorno == 0)
return;

v8::Local<v8::Object> obj = object.As<v8::Object>();
const char* err_string = node::errno_string(errorno);

if (message == nullptr || message[0] == '\0') {
message = strerror(errorno);
}

node::CollectExceptionInfo(this, obj, errorno, err_string,
syscall, message, path, nullptr);
}

void Environment::CollectUVExceptionInfo(v8::Local<v8::Value> object,
int errorno,
const char* syscall,
const char* message,
const char* path,
const char* dest) {
if (!object->IsObject() || errorno == 0)
return;

v8::Local<v8::Object> obj = object.As<v8::Object>();
const char* err_string = uv_err_name(errorno);

if (message == nullptr || message[0] == '\0') {
message = uv_strerror(errorno);
}

node::CollectExceptionInfo(this, obj, errorno, err_string,
syscall, message, path, dest);
}

} // namespace node
Loading