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.

Sign up for GitHub

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

timers, errors: migrate to internal/errors.js #11384

Closed
wants to merge 5 commits into from
Closed

timers, errors: migrate to internal/errors.js #11384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad8129e
doc: add docs for new errors
shubheksha Feb 14, 2017
59d4218
errors: add new error types
shubheksha Feb 14, 2017
ff6da8d
timers, test: migrate to internal/errors.js
shubheksha Feb 14, 2017
2a2fb41
test: refactor functions to use arrow functions
shubheksha Feb 14, 2017
8b929c5
Fix leftover issues after rebase
shubheksha Jun 16, 2017
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
8 changes: 8 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,9 @@ found [here][online].
the connected party did not properly respond after a period of time. Usually
encountered by [`http`][] or [`net`][] -- often a sign that a `socket.end()`
was not properly called.

<a id="ERROR_CODES"></a>
### ERROR CODES

<a id="nodejs-error-codes"></a>
## Node.js Error Codes
Expand Down Expand Up @@ -585,6 +588,9 @@ API when a callbackified `Promise` is rejected with a falsy value (e.g. `null`).

Used when a given index is out of the accepted range.

<a id="nodejs-error-codes"></a>
## Node.js Error Codes

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

Expand Down Expand Up @@ -799,6 +805,7 @@ are most likely an indication of a bug within Node.js itself.
[`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
[`child.kill()`]: child_process.html#child_process_child_kill_signal
[`child.send()`]: child_process.html#child_process_child_send_message_sendhandle_options_callback
[`fs.readdir`]: fs.html#fs_fs_readdir_path_options_callback
[`fs.readFileSync`]: fs.html#fs_fs_readfilesync_file_options
[`fs.readdir`]: fs.html#fs_fs_readdir_path_options_callback
[`fs.unlink`]: fs.html#fs_fs_unlink_path_callback
Expand All @@ -817,6 +824,7 @@ are most likely an indication of a bug within Node.js itself.
[domains]: domain.html
[event emitter-based]: events.html#events_class_eventemitter
[file descriptors]: https://en.wikipedia.org/wiki/File_descriptor
[Node.js Error Codes]: #nodejs-error-codes
[online]: http://man7.org/linux/man-pages/man3/errno.3.html
[stream-based]: stream.html
[syscall]: http://man7.org/linux/man-pages/man2/syscall.2.html
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ E('ERR_V8BREAKITERATOR', 'full ICU data not installed. ' +
'See /~https://github.com/nodejs/node/wiki/Intl');
// Add new errors from here...

E('ERR_INVALID_ARG_TYPE', invalidArgType);
E('ERR_INVALID_CALLBACK', 'callback must be a function');

function invalidArgType(name, expected, actual) {
const assert = lazyAssert();
assert(name, 'name is required');
Expand Down Expand Up @@ -227,4 +230,4 @@ function oneOf(expected, thing) {
} else {
return `of ${thing} ${String(expected)}`;
}
}
}
12 changes: 7 additions & 5 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const assert = require('assert');
const util = require('util');
const debug = util.debuglog('timer');
const kOnTimeout = TimerWrap.kOnTimeout | 0;

const initTriggerId = async_hooks.initTriggerId;
// Two arrays that share state between C++ and JS.
const { async_hook_fields, async_uid_fields } = async_wrap;
Expand All @@ -45,6 +46,7 @@ const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr } =
const async_id_symbol = Symbol('asyncId');
const trigger_id_symbol = Symbol('triggerAsyncId');

const errors = require('internal/errors');
// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2147483647; // 2^31-1

Expand Down Expand Up @@ -408,11 +410,11 @@ const unenroll = exports.unenroll = function(item) {
// Using existing objects as timers slightly reduces object overhead.
exports.enroll = function(item, msecs) {
if (typeof msecs !== 'number') {
throw new TypeError('"msecs" argument must be a number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs', 'number');
}

if (msecs < 0 || !isFinite(msecs)) {
throw new RangeError('"msecs" argument must be ' +
throw new errors.RangeError('ERR_INVALID_ARG_TYPE', 'msecs',
'a non-negative finite number');
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. will have to think about this one because the error message is not going to format that well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have separate error statements for non-finite and negative numbers

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It would be a separate error code but that's workable

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It would be a separate error code but that's workable

Copy link
Member

Choose a reason for hiding this comment

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

Note: both #11400 and #11390 touch this kind of error. Definitely +1 on having a error code dedicated for it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misunderstood the discussion, it was about throwing different errors for <0 and isFinite, sorry. Anyway I still think there should be something like ERR_NEGATIVE_ARG

Copy link
Member

Choose a reason for hiding this comment

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

@shubheksha do you want to take a stab at addressing @joyeecheung 's comment? Thanks!

}

Expand All @@ -437,7 +439,7 @@ exports.enroll = function(item, msecs) {

function setTimeout(callback, after, arg1, arg2, arg3) {
if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

var len = arguments.length;
Expand Down Expand Up @@ -534,7 +536,7 @@ const clearTimeout = exports.clearTimeout = function(timer) {

exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

var len = arguments.length;
Expand Down Expand Up @@ -829,7 +831,7 @@ function Immediate() {

function setImmediate(callback, arg1, arg2, arg3) {
if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

var i, args;
Expand Down
22 changes: 11 additions & 11 deletions test/parallel/test-net-socket-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,27 @@ const validDelays = [0, 0.001, 1, 1e6];


for (let i = 0; i < nonNumericDelays.length; i++) {
assert.throws(function() {
s.setTimeout(nonNumericDelays[i], common.noop);
}, TypeError);
assert.throws(() => {
s.setTimeout(nonNumericDelays[i], noop);
}, common.expectsError('ERR_INVALID_ARG_TYPE', TypeError));
}

for (let i = 0; i < badRangeDelays.length; i++) {
assert.throws(function() {
s.setTimeout(badRangeDelays[i], common.noop);
}, RangeError);
assert.throws(() => {
s.setTimeout(badRangeDelays[i], noop);
}, common.expectsError('ERR_INVALID_ARG_TYPE', RangeError));
}

for (let i = 0; i < validDelays.length; i++) {
assert.doesNotThrow(function() {
s.setTimeout(validDelays[i], common.noop);
assert.doesNotThrow(() => {
s.setTimeout(validDelays[i], noop);
});
}

const server = net.Server();
server.listen(0, common.mustCall(function() {
const socket = net.createConnection(this.address().port);
socket.setTimeout(1, common.mustCall(function() {
server.listen(0, common.mustCall(() => {
const socket = net.createConnection(server.address().port);
socket.setTimeout(1, common.mustCall(() => {
socket.destroy();
server.close();
}));
Expand Down
40 changes: 21 additions & 19 deletions test/parallel/test-timers-throw-when-cb-not-function.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';
require('../common');
const common = require('../common');
const assert = require('assert');

function doSetTimeout(callback, after) {
Expand All @@ -8,18 +8,20 @@ function doSetTimeout(callback, after) {
};
}

const expectedError = common.expectsError('ERR_INVALID_CALLBACK', TypeError);

assert.throws(doSetTimeout('foo'),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetTimeout({foo: 'bar'}),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetTimeout(),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetTimeout(undefined, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetTimeout(null, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetTimeout(false, 0),
/"callback" argument must be a function/);
expectedError);


function doSetInterval(callback, after) {
Expand All @@ -29,17 +31,17 @@ function doSetInterval(callback, after) {
}

assert.throws(doSetInterval('foo'),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetInterval({foo: 'bar'}),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetInterval(),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetInterval(undefined, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetInterval(null, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetInterval(false, 0),
/"callback" argument must be a function/);
expectedError);


function doSetImmediate(callback, after) {
Expand All @@ -49,14 +51,14 @@ function doSetImmediate(callback, after) {
}

assert.throws(doSetImmediate('foo'),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetImmediate({foo: 'bar'}),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetImmediate(),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetImmediate(undefined, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetImmediate(null, 0),
/"callback" argument must be a function/);
expectedError);
assert.throws(doSetImmediate(false, 0),
/"callback" argument must be a function/);
expectedError);