events: fix TypeError in EventEmitter warning #9021

jseagull · 2016-10-11T05:22:10Z

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott

LGTM if CI is OK.

Nit: Would be great if you could rewrite the commit message to follow the commit message format requested in the CONTRIBUTING.md doc.

/cc @nodejs/testing At first I thought the test file where the test is put was probably not the right place, based on the test file name, but closer inspection makes me think it's actually OK. Second opinion welcome, though.

lpinca · 2016-10-11T06:09:44Z

@jseagull I would add another test, similar to this one https://github.com/nodejs/node/blob/master/test/parallel/test-event-emitter-max-listeners-warning.js, to check that error message is the expected one.

jseagull · 2016-10-11T06:57:48Z

@lpinca
test added

lpinca · 2016-10-11T07:06:35Z

test/parallel/test-event-emitter-max-listeners-warning-for-symbol.js

+e.setMaxListeners(1);
+
+process.on('warning', common.mustCall((warning) => {
+  assert.ok(warning instanceof Error);


Can you add an assertion with something like this:

assert.ok(warning.message.includes('2 Symbol(symbol) listeners added.'));

santigimeno · 2016-10-11T07:21:28Z

LGTM with the commit message format fixed, @lpinca suggestion implemented and CI green.

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

santigimeno · 2016-10-11T07:22:58Z

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

lpinca

LGTM

mscdex · 2016-10-11T07:54:23Z

lib/events.js

@@ -257,8 +257,9 @@ function _addListener(target, type, listener, prepend) {
      if (m && m > 0 && existing.length > m) {
        existing.warned = true;
        const w = new Error('Possible EventEmitter memory leak detected. ' +
-                            `${existing.length} ${type} listeners added. ` +
-                            'Use emitter.setMaxListeners() to increase limit');
+                            `${existing.length} ${type.toString()} listeners ` +


This isn't safe since type can currently be any type of value. So null.toString() will fail for example. It will probably be better to just concatenate with an empty string.

EDIT: On second thought, we may have to just add logic before this to figure out how to create presentable output since Symbols cannot be implicitly converted. Perhaps something like this?:

var typeStr; if (type && typeof type.toString === 'function') typeStr = type.toString(); else typeStr = '' + type; const w = new Error('Possible EventEmitter memory leak detected. ' + `${existing.length} ${typeStr} listeners ` + // ...

@mscdex isn't this a programmer error? type should be either a string or a symbol.

@mscdex I change type.toString() to String(type) for safe reason

@lpinca No, currently values like null are allowed as types. I've actually used numbers before for the event name as part of a generic protocol parser.

gibfahn · 2016-10-11T09:41:58Z

For the commit message, maybe something like:

events,test: fix TypeError in EventEmitter warning

Allows Symbol to be converted to String so it can be included in the 
error.

Fixes: https://github.com/nodejs/node/issues/9003

gibfahn · 2016-10-11T09:43:04Z

cc/ @addaleax @jasnell as the last people to work on this code.

addaleax

LGTM. The ,test in the commit message can be left out, ideally almost all code changes come with tests, so that can be assumed as an implied default :)

addaleax · 2016-10-11T10:05:33Z

test/parallel/test-event-emitter-check-listener-leaks.js

@@ -13,6 +13,14 @@ assert.ok(!e._events['default'].hasOwnProperty('warned'));
 e.on('default', function() {});
 assert.ok(e._events['default'].warned);

+// symbol
+var sym = Symbol('x');


tiny nit: This can be const

All variable in this file is defined with var, so I kept the style.

We are in the process of migrating the code from var to const. It's not really practical to update a codebase this large in one pass, so we make the updates as we touch the code. Can you please add new code using const.

If you use const and let, you can also isolate the tests using curly braces. So:

{ const sym = Symbol('x'); e.setMaxListeners(1); ... }

lpinca · 2016-10-11T12:08:59Z

I took the liberty to update PR title.

jasnell

LGTM

jasnell · 2016-10-11T17:04:41Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/4458/
Lots of red in the last run.

Trott · 2016-10-11T20:30:30Z

Nit: Add a test for null to cover the use case described by @mscdex.

Allows Symbol to be converted to String so it can be included in the error. Fixes: nodejs#9003

jseagull · 2016-10-12T03:04:31Z

@jasnell

Using 'Changelog to branch' strategy.
Cleaning workspace
 > git rev-parse --verify HEAD # timeout=10
Resetting working tree
 > git reset --hard # timeout=10
FATAL: java.io.EOFException
hudson.remoting.RequestAbortedException: java.io.EOFException
    at hudson.remoting.Request.abort(Request.java:303)
    at hudson.remoting.Channel.terminate(Channel.java:847)
    at org.jenkinsci.remoting.nio.NioChannelHub$3.run(NioChannelHub.java:614)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
    at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
    at ......remote call to test-digitalocean-freebsd10-x64-1(Native Method)
    at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1416)
    at hudson.remoting.Request.call(Request.java:172)
    at hudson.remoting.Channel.call(Channel.java:780)
    at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:249)
    at com.sun.proxy.$Proxy73.clean(Unknown Source)
    at org.jenkinsci.plugins.gitclient.RemoteGitImpl.clean(RemoteGitImpl.java:449)
    at hudson.plugins.git.extensions.impl.CleanCheckout.onCheckoutCompleted(CleanCheckout.java:28)
    at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1137)
    at hudson.scm.SCM.checkout(SCM.java:485)
    at hudson.model.AbstractProject.checkout(AbstractProject.java:1269)
    at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:607)
    at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
    at hudson.model.Run.execute(Run.java:1738)
    at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:410)
Caused by: java.io.EOFException
    at org.jenkinsci.remoting.nio.NioChannelHub$3.run(NioChannelHub.java:614)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
    at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Run condition [Always] enabling perform for step [[Execute a set of scripts, Execute a set of scripts]]
ERROR: Step ?Publish TAP Results? failed: no workspace for node-test-commit-freebsd/nodes=freebsd10-64 #4743
Checking ^not ok
ERROR: Build step failed with exception
java.lang.NullPointerException
    at hudson.plugins.textfinder.TextFinderPublisher.findText(TextFinderPublisher.java:101)
    at hudson.plugins.textfinder.TextFinderPublisher.perform(TextFinderPublisher.java:74)
    at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
    at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:782)
    at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:723)
    at hudson.model.Build$BuildExecution.post2(Build.java:185)
    at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:668)
    at hudson.model.Run.execute(Run.java:1763)
    at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:410)
Build step 'Jenkins Text Finder' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

It seems this is not caused by the changed code.

jseagull · 2016-10-12T03:05:35Z

@Trott covered

jasnell · 2016-10-19T16:07:12Z

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

Allows Symbol to be converted to String so it can be included in the error. Fixes: #9003 PR-URL: #9021 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

jasnell · 2016-10-26T20:04:53Z

Landed in 254ab63

Allows Symbol to be converted to String so it can be included in the error. Fixes: #9003 PR-URL: #9021 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

MylesBorins · 2016-11-22T07:09:43Z

should this be backported? If so it will need a PR for both v4.x and v6.x

gibfahn · 2017-04-16T13:48:43Z

@jseagull if you'd be willing to raise a backport PR, the guide is here. Otherwise someone (anyone who's willing!) else can raise one.

Allows Symbol to be converted to String so it can be included in the error. Conflicts: lib/events.js Fixes: #9003 Backport-PR-URL: #12459 PR-URL: #9021 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Allows Symbol to be converted to String so it can be included in the error. Conflicts: lib/events.js Fixes: nodejs#9003 Backport-PR-URL: nodejs/node#12459 PR-URL: nodejs/node#9021 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 11, 2016

Trott approved these changes Oct 11, 2016

View reviewed changes

jseagull force-pushed the master branch from f02b64c to d386b0d Compare October 11, 2016 06:55

lpinca reviewed Oct 11, 2016

View reviewed changes

jseagull force-pushed the master branch from d386b0d to 89582a7 Compare October 11, 2016 07:21

lpinca approved these changes Oct 11, 2016

View reviewed changes

mscdex reviewed Oct 11, 2016

View reviewed changes

jseagull force-pushed the master branch from 89582a7 to 8a4e52c Compare October 11, 2016 08:23

jseagull force-pushed the master branch from 8a4e52c to d03d4e4 Compare October 11, 2016 10:00

addaleax approved these changes Oct 11, 2016

View reviewed changes

lpinca changed the title ~~Fix https://github.com/nodejs/node/issues/9003 and add test~~ events: fix TypeError in EventEmitter warning Oct 11, 2016

jasnell approved these changes Oct 11, 2016

View reviewed changes

jasnell added the error-handling label Oct 11, 2016

thefourtheye approved these changes Oct 11, 2016

View reviewed changes

jasnell self-assigned this Oct 11, 2016

events,test: fix TypeError in EventEmitter warning

6434859

Allows Symbol to be converted to String so it can be included in the error. Fixes: nodejs#9003

jseagull force-pushed the master branch from d03d4e4 to 6434859 Compare October 12, 2016 02:53

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell closed this Oct 26, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added lts-watch-v4.x labels Nov 22, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Apr 13, 2017

gibfahn mentioned this pull request Apr 16, 2017

TypeError: EventEmitter, setMaxListeners, Symbol #9003

Closed

gibfahn mentioned this pull request Apr 17, 2017

(v6.x backport) events,test: fix TypeError in EventEmitter warning #12459

Closed

3 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

BridgeAR mentioned this pull request Jan 20, 2020

events: add type check for event name #31428

Closed

3 tasks

aliscco mentioned this pull request Dec 1, 2023

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#903

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#508

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: fix TypeError in EventEmitter warning #9021

events: fix TypeError in EventEmitter warning #9021

jseagull commented Oct 11, 2016

Trott left a comment

lpinca commented Oct 11, 2016

jseagull commented Oct 11, 2016

lpinca Oct 11, 2016

santigimeno commented Oct 11, 2016

santigimeno commented Oct 11, 2016

lpinca left a comment

mscdex Oct 11, 2016 •

edited

Loading

lpinca Oct 11, 2016

jseagull Oct 11, 2016

mscdex Oct 11, 2016

lpinca Oct 11, 2016

gibfahn commented Oct 11, 2016

gibfahn commented Oct 11, 2016

addaleax left a comment

addaleax Oct 11, 2016

jseagull Oct 11, 2016

cjihrig Oct 11, 2016

cjihrig Oct 11, 2016

lpinca commented Oct 11, 2016

jasnell left a comment

jasnell commented Oct 11, 2016 •

edited

Loading

Trott commented Oct 11, 2016

jseagull commented Oct 12, 2016 •

edited

Loading

jseagull commented Oct 12, 2016

jasnell commented Oct 19, 2016

jasnell commented Oct 26, 2016

MylesBorins commented Nov 22, 2016

gibfahn commented Apr 16, 2017

events: fix TypeError in EventEmitter warning #9021

events: fix TypeError in EventEmitter warning #9021

Conversation

jseagull commented Oct 11, 2016

Trott left a comment

Choose a reason for hiding this comment

lpinca commented Oct 11, 2016

jseagull commented Oct 11, 2016

Choose a reason for hiding this comment

santigimeno commented Oct 11, 2016

santigimeno commented Oct 11, 2016

lpinca left a comment

Choose a reason for hiding this comment

mscdex Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Oct 11, 2016

gibfahn commented Oct 11, 2016

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Oct 11, 2016

jasnell left a comment

Choose a reason for hiding this comment

jasnell commented Oct 11, 2016 • edited Loading

Trott commented Oct 11, 2016

jseagull commented Oct 12, 2016 • edited Loading

jseagull commented Oct 12, 2016

jasnell commented Oct 19, 2016

jasnell commented Oct 26, 2016

MylesBorins commented Nov 22, 2016

gibfahn commented Apr 16, 2017

mscdex Oct 11, 2016 •

edited

Loading

jasnell commented Oct 11, 2016 •

edited

Loading

jseagull commented Oct 12, 2016 •

edited

Loading