events, doc: add type checking in defaultMaxListeners getter #11938

DavidCai1111 · 2017-03-20T13:59:34Z

Since EventEmitter.defaultMaxListeners and emitter.setMaxListeners(n) are both public APIs listed in the document, so it is reasonable to check EventEmitter.defaultMaxListeners getter's input like what emitter.setMaxListeners(n) does to avoid potential confusing results or error messages caused by some userland mistaken input, like:

'use strict'
const E = require('events')

E.defaultMaxListeners = new Proxy({}, { get: () => {} })
let e = new E()
e.addListener('1', function () {})
e.addListener('1', function () {})
// output:
// events.js:260
//      if (m && m > 0 && existing.length > m) {
//                 ^
//
// TypeError: Cannot convert object to primitive value
//     at _addListener (events.js:260:18)
//     at EventEmitter.addListener (events.js:279:10)
//     at Object.<anonymous> (/Users/caiwei/workspace/benchmarks/index.js:7:3)
//     at Module._compile (module.js:571:32)
//     at Object.Module._extensions..js (module.js:580:10)
//     at Module.load (module.js:488:32)
//     at tryModuleLoad (module.js:447:12)
//     at Function.Module._load (module.js:439:3)
//     at Module.runMain (module.js:605:10)
//     at run (bootstrap_node.js:425:7)

Checklist

[-] make -j4 test (UNIX), or vcbuild test (Windows) passes
[-] tests and/or benchmarks are included
[-] documentation is changed or added
[-] commit message follows commit guidelines

Affected core subsystem(s)

events, doc

mscdex · 2017-03-20T14:18:30Z

lib/events.js

    // force global console to be compiled.
    // see https://github.com/nodejs/node/issues/4467
    console;
-    defaultMaxListeners = arg;
+    if (typeof n !== 'number' || n < 0 || isNaN(n))


Could we replace isNaN(n) with n !== n, possibly with a comment with a short explanation?

@mscdex Is this similar place also worth replacing then?

Probably, but as a separate commit I'd say.

@mscdex @vsemozhetbyt OK, updated.

DavidCai1111 · 2017-03-20T14:37:49Z

@vsemozhetbyt Thanks for reminding, changed it to a shorter one :)

vsemozhetbyt · 2017-03-20T15:08:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6932/

vsemozhetbyt · 2017-03-20T15:45:10Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/6934/

mscdex · 2017-03-20T16:17:40Z

lib/events.js

    // force global console to be compiled.
    // see https://github.com/nodejs/node/issues/4467
    console;
-    defaultMaxListeners = arg;
+    // check whether the input is a positive number (which value is
+    // larger than zero and not a NaN).


'which value is larger than zero' should probably instead be something like 'whose value is zero or greater'

cjihrig

LGTM with a couple nits.

cjihrig · 2017-03-20T17:00:23Z

doc/api/events.md

@@ -262,7 +262,8 @@ By default, a maximum of `10` listeners can be registered for any single
 event. This limit can be changed for individual `EventEmitter` instances
 using the [`emitter.setMaxListeners(n)`][] method. To change the default
 for *all* `EventEmitter` instances, the `EventEmitter.defaultMaxListeners`
-property can be used.
+property can be used. If trying to set it to a value which is not a positive


Can I suggest rewording this as:

If this value is not a positive number, a TypeError will be thrown.

cjihrig · 2017-03-20T17:01:32Z

lib/events.js

@@ -52,11 +52,15 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
  get: function() {
    return defaultMaxListeners;
  },
-  set: function(arg) {
+  set: function(n) {


Changing this variable name seems unnecessary.

jasnell

LGTM with nits addressed

DavidCai1111 · 2017-03-21T00:24:58Z

@mscdex @cjihrig Thanks for reviewing. Just updated, PTAL. 😃

mscdex · 2017-03-21T02:25:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6948/

jasnell · 2017-03-22T16:33:01Z

@mscdex ... does this LGTY?

mscdex · 2017-03-22T16:42:31Z

lib/events.js

+    // 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');


'defaultMaxListeners' should be enclosed in double quotes to match the style when referencing property names in error messages.

mscdex · 2017-03-22T16:43:52Z

One minor nit, but otherwise LGTM.

jasnell · 2017-03-22T20:56:14Z

I can take care of that nit while landing

PR-URL: #11938 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2017-03-22T21:04:45Z

Landed in 221b03a

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Mar 20, 2017

mscdex reviewed Mar 20, 2017

View reviewed changes

DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch from aa568e6 to 58f7610 Compare March 20, 2017 14:33

DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch 2 times, most recently from ce92c12 to 35db7e6 Compare March 20, 2017 15:35

mscdex reviewed Mar 20, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 20, 2017

cjihrig approved these changes Mar 20, 2017

View reviewed changes

jasnell approved these changes Mar 20, 2017

View reviewed changes

lpinca approved these changes Mar 20, 2017

View reviewed changes

hiroppy approved these changes Mar 20, 2017

View reviewed changes

events, doc: check input in defaultMaxListeners

b3c06f2

DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch from 35db7e6 to b3c06f2 Compare March 21, 2017 00:20

mscdex reviewed Mar 22, 2017

View reviewed changes

jasnell closed this Mar 22, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events, doc: add type checking in defaultMaxListeners getter #11938

events, doc: add type checking in defaultMaxListeners getter #11938

DavidCai1111 commented Mar 20, 2017 •

edited

Loading

mscdex Mar 20, 2017

vsemozhetbyt Mar 20, 2017 •

edited

Loading

mscdex Mar 20, 2017

DavidCai1111 Mar 20, 2017

DavidCai1111 commented Mar 20, 2017

vsemozhetbyt commented Mar 20, 2017

vsemozhetbyt commented Mar 20, 2017

mscdex Mar 20, 2017

cjihrig left a comment

cjihrig Mar 20, 2017

cjihrig Mar 20, 2017

jasnell left a comment

DavidCai1111 commented Mar 21, 2017

mscdex commented Mar 21, 2017

jasnell commented Mar 22, 2017

mscdex Mar 22, 2017 •

edited

Loading

mscdex commented Mar 22, 2017

jasnell commented Mar 22, 2017

jasnell commented Mar 22, 2017

events, doc: add type checking in defaultMaxListeners getter #11938

events, doc: add type checking in defaultMaxListeners getter #11938

Conversation

DavidCai1111 commented Mar 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex Mar 20, 2017

Choose a reason for hiding this comment

vsemozhetbyt Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex Mar 20, 2017

Choose a reason for hiding this comment

DavidCai1111 Mar 20, 2017

Choose a reason for hiding this comment

DavidCai1111 commented Mar 20, 2017

vsemozhetbyt commented Mar 20, 2017

vsemozhetbyt commented Mar 20, 2017

mscdex Mar 20, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Mar 20, 2017

Choose a reason for hiding this comment

cjihrig Mar 20, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

DavidCai1111 commented Mar 21, 2017

mscdex commented Mar 21, 2017

jasnell commented Mar 22, 2017

mscdex Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Mar 22, 2017

jasnell commented Mar 22, 2017

jasnell commented Mar 22, 2017

DavidCai1111 commented Mar 20, 2017 •

edited

Loading

vsemozhetbyt Mar 20, 2017 •

edited

Loading

mscdex Mar 22, 2017 •

edited

Loading