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

events: migrate to internal/errors #15623

Closed
wants to merge 1 commit 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
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1251,6 +1251,12 @@ buffer.

Used when a string that contains unescaped characters was received.

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

Used when an unhandled "error" occurs (for instance, when an `'error'` event
is emitted by an `EventEmitter` but an `'error'` handler is not registered).

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

Expand Down
55 changes: 40 additions & 15 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ EventEmitter.prototype._maxListeners = undefined;
// added to it. This is a useful default which helps finding memory leaks.
var defaultMaxListeners = 10;

var errors;
function lazyErrors() {
if (errors === undefined)
errors = require('internal/errors');
return errors;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to lazily load this? Is this the pattern used everywhere? Should we?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because events.js is loaded before all the other modules in the dependency tree (because of bootstrapping process. Lazy loading errors prevents errors from being loaded before events is loaded.

Copy link
Member

Choose a reason for hiding this comment

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

I would still rather inline this instead of using the function form but it should not be a blocker.


Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
enumerable: true,
get: function() {
Expand All @@ -52,8 +59,10 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
console;
// check whether the input is a positive number (whose value is zero or
// greater and not a NaN).
if (typeof arg !== 'number' || arg < 0 || arg !== arg)
throw new TypeError('"defaultMaxListeners" must be a positive number');
if (typeof arg !== 'number' || arg < 0 || arg !== arg) {
const errors = lazyErrors();
throw new errors.TypeError('ERR_OUT_OF_RANGE', 'defaultMaxListeners');
}
defaultMaxListeners = arg;
}
});
Expand All @@ -79,8 +88,10 @@ EventEmitter.init = function() {
// Obviously not all Emitters should be limited to 10. This function allows
// that to be increased. Set to zero for unlimited.
EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) {
if (typeof n !== 'number' || n < 0 || isNaN(n))
throw new TypeError('"n" argument must be a positive number');
if (typeof n !== 'number' || n < 0 || isNaN(n)) {
const errors = lazyErrors();
throw new errors.TypeError('ERR_OUT_OF_RANGE', 'n');
}
this._maxListeners = n;
return this;
};
Expand Down Expand Up @@ -170,8 +181,10 @@ EventEmitter.prototype.emit = function emit(type) {
if (arguments.length > 1)
er = arguments[1];
if (domain) {
if (!er)
er = new Error('Unhandled "error" event');
if (!er) {
const errors = lazyErrors();
er = new errors.Error('ERR_UNHANDLED_ERROR');
}
if (typeof er === 'object' && er !== null) {
er.domainEmitter = this;
er.domain = domain;
Expand All @@ -182,7 +195,8 @@ EventEmitter.prototype.emit = function emit(type) {
throw er; // Unhandled 'error' event
} else {
// At least give some kind of context to the user
const err = new Error('Unhandled "error" event. (' + er + ')');
const errors = lazyErrors();
const err = new errors.Error('ERR_UNHANDLED_ERROR', er);
err.context = er;
throw err;
}
Expand Down Expand Up @@ -234,8 +248,10 @@ function _addListener(target, type, listener, prepend) {
var events;
var existing;

if (typeof listener !== 'function')
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener', 'function');
}

events = target._events;
if (!events) {
Expand Down Expand Up @@ -278,6 +294,7 @@ function _addListener(target, type, listener, prepend) {
m = $getMaxListeners(target);
if (m && m > 0 && existing.length > m) {
existing.warned = true;
// No error code for this since it is a Warning
const w = new Error('Possible EventEmitter memory leak detected. ' +
`${existing.length} ${String(type)} listeners ` +
'added. Use emitter.setMaxListeners() to ' +
Expand Down Expand Up @@ -337,16 +354,21 @@ function _onceWrap(target, type, listener) {
}

EventEmitter.prototype.once = function once(type, listener) {
if (typeof listener !== 'function')
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener', 'function');
}
this.on(type, _onceWrap(this, type, listener));
return this;
};

EventEmitter.prototype.prependOnceListener =
function prependOnceListener(type, listener) {
if (typeof listener !== 'function')
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener',
'function');
}
this.prependListener(type, _onceWrap(this, type, listener));
return this;
};
Expand All @@ -356,8 +378,11 @@ EventEmitter.prototype.removeListener =
function removeListener(type, listener) {
var list, events, position, i, originalListener;

if (typeof listener !== 'function')
throw new TypeError('"listener" argument must be a function');
if (typeof listener !== 'function') {
const errors = lazyErrors();
Copy link
Member

Choose a reason for hiding this comment

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

Do you know where the circular reference is coming from? If so - is there any possibility in untying that?
I personally also prefer using a simple if a lot over the function version. It is still a minor overhead of using the function while there is no faster check than a simple if to undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a circular reference that is the concern. The events module is one of the very first modules loaded in core (because it is needed by process) and as such needs to be loaded before anything else is loaded.

throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'listener',
'function');
}

events = this._events;
if (!events)
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,12 @@ E('ERR_TRANSFORM_WITH_LENGTH_0',
'Calling transform done when writableState.length != 0');
E('ERR_UNESCAPED_CHARACTERS',
(name) => `${name} contains unescaped characters`);
E('ERR_UNHANDLED_ERROR',
(err) => {
const msg = 'Unhandled error.';
if (err === undefined) return msg;
return `${msg} (${err})`;
});
E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s');
E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: %s');
E('ERR_UNKNOWN_MODULE_FORMAT', 'Unknown module format: %s');
Expand Down
9 changes: 6 additions & 3 deletions test/parallel/test-event-emitter-add-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,11 @@ const EventEmitter = require('events');
}

// Verify that the listener must be a function
assert.throws(() => {
common.expectsError(() => {
const ee = new EventEmitter();

ee.on('foo', null);
}, /^TypeError: "listener" argument must be a function$/);
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "listener" argument must be of type function'
});
25 changes: 17 additions & 8 deletions test/parallel/test-event-emitter-errors.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
'use strict';
require('../common');
const common = require('../common');
const EventEmitter = require('events');
const assert = require('assert');

const EE = new EventEmitter();

assert.throws(() => {
EE.emit('error', 'Accepts a string');
}, /^Error: Unhandled "error" event\. \(Accepts a string\)$/);
common.expectsError(
() => EE.emit('error', 'Accepts a string'),
{
code: 'ERR_UNHANDLED_ERROR',
type: Error,
message: 'Unhandled error. (Accepts a string)'
Copy link
Member

Choose a reason for hiding this comment

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

The message looks somewhat weird here because of the dot before the brackets.
I think it would be fine to either remove the dot or to move it to the end of the string.

}
);

assert.throws(() => {
EE.emit('error', { message: 'Error!' });
}, /^Error: Unhandled "error" event\. \(\[object Object\]\)$/);
common.expectsError(
() => EE.emit('error', { message: 'Error!' }),
{
code: 'ERR_UNHANDLED_ERROR',
type: Error,
message: 'Unhandled error. ([object Object])'
}
);
22 changes: 17 additions & 5 deletions test/parallel/test-event-emitter-max-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

'use strict';
const common = require('../common');
const assert = require('assert');
const events = require('events');
const e = new events.EventEmitter();

Expand All @@ -31,12 +30,25 @@ e.on('maxListeners', common.mustCall());
e.setMaxListeners(42);

const throwsObjs = [NaN, -1, 'and even this'];
const maxError = /^TypeError: "n" argument must be a positive number$/;
const defError = /^TypeError: "defaultMaxListeners" must be a positive number$/;

for (const obj of throwsObjs) {
assert.throws(() => e.setMaxListeners(obj), maxError);
assert.throws(() => events.defaultMaxListeners = obj, defError);
common.expectsError(
() => e.setMaxListeners(obj),
{
code: 'ERR_OUT_OF_RANGE',
type: TypeError,
message: 'The "n" argument is out of range'
}
);

common.expectsError(
() => events.defaultMaxListeners = obj,
{
code: 'ERR_OUT_OF_RANGE',
type: TypeError,
message: 'The "defaultMaxListeners" argument is out of range'
}
);
}

e.emit('maxListeners');
8 changes: 6 additions & 2 deletions test/parallel/test-event-emitter-once.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,15 @@ e.once('e', common.mustCall());
e.emit('e');

// Verify that the listener must be a function
assert.throws(() => {
common.expectsError(() => {
const ee = new EventEmitter();

ee.once('foo', null);
}, /^TypeError: "listener" argument must be a function$/);
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "listener" argument must be of type function'
});

{
// once() has different code paths based on the number of arguments being
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-event-emitter-prepend.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@ myEE.prependOnceListener('foo',
myEE.emit('foo');

// Verify that the listener must be a function
assert.throws(() => {
common.expectsError(() => {
const ee = new EventEmitter();

ee.prependOnceListener('foo', null);
}, /^TypeError: "listener" argument must be a function$/);
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "listener" argument must be of type function'
});

// Test fallback if prependListener is undefined.
const stream = require('stream');
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-event-emitter-remove-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,15 @@ function listener2() {}
}

// Verify that the removed listener must be a function
assert.throws(() => {
common.expectsError(() => {
const ee = new EventEmitter();

ee.removeListener('foo', null);
}, /^TypeError: "listener" argument must be a function$/);
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "listener" argument must be of type function'
});

{
const ee = new EventEmitter();
Expand Down