events: update and clarify error message #10387

ctide · 2016-12-21T17:51:08Z

eventemitter: clarify error message

Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

eventemitter

Description of change

Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.

mscdex · 2016-12-21T19:15:39Z

lib/events.js

@@ -160,7 +160,7 @@ EventEmitter.prototype.emit = function emit(type) {
      throw er; // Unhandled 'error' event
    } else {
      // At least give some kind of context to the user
-      var err = new Error('Uncaught, unspecified "error" event. (' + er + ')');
+      var err = new Error('An "error" event was emitted, but no listeners are attached to this emitter: (' + er + ')');


This line is too long.

mscdex · 2016-12-21T19:15:57Z

lib/events.js

@@ -151,7 +151,7 @@ EventEmitter.prototype.emit = function emit(type) {
    er = arguments[1];
    if (domain) {
      if (!er)
-        er = new Error('Uncaught, unspecified "error" event');
+        er = new Error('An "error" event was emitted, but no listeners are attached to this emitter.');


This line is too long.

sam-github

Unspecified is confusing, yes, but I don't think the text is an improvement. I can think of 2 other ways to avoid the abort that don't involve listening for the error (the 'uncaughtException' handler and domains), so the text is both too long, and not correct.

How about just 'Uncaught "error" event'?

ctide · 2016-12-21T19:36:41Z

Fixed, sorry, didn't realize that running tests didn't run the linter as well.

ctide · 2016-12-21T19:37:39Z

Sure, that's reasonable. Unspecified was what really threw me off. I'll update and rebase.

sam-github · 2016-12-21T19:39:09Z

Also, needs a unit test - and yes, I know, it should have already had one, sorry!

ctide · 2016-12-21T19:54:38Z

Seemed like a message was the cleanest way to add a test for this. If that's not the right approach, let me know and I can update!

cjihrig · 2016-12-21T19:57:16Z

test/message/unhandled_eventemitter_exception.out

+      throw err;
+      ^
+
+Error: Uncaught "error" event: ([object Object])


[object Object] is probably not the desired output.

cjihrig · 2016-12-21T19:57:56Z

You don't need a message test. There is almost certainly a test that already exercises this behavior. You would just need to add an assertion on the error message in such a test.

ctide · 2016-12-21T20:06:54Z

OK, found the real test and just updated the regex to include the changes.

mscdex · 2016-12-21T20:11:49Z

lib/events.js

@@ -160,7 +160,7 @@ EventEmitter.prototype.emit = function emit(type) {
      throw er; // Unhandled 'error' event
    } else {
      // At least give some kind of context to the user
-      var err = new Error('Uncaught, unspecified "error" event. (' + er + ')');
+      var err = new Error('Uncaught "error" event: (' + er + ')');


I think either the parens should be dropped or the colon removed.

mscdex · 2016-12-21T20:14:15Z

IMHO I think "uncaught" is a bit of a misnomer, since to me it sounds like the error was thrown because of a missing try-catch or something. What about "unhandled" instead?

ctide · 2016-12-21T20:48:59Z

Yeah, unhandled is more accurate than uncaught. Will push an update in a minute that changes it to that.

cjihrig · 2016-12-21T20:58:51Z

test/parallel/test-event-emitter-errors.js

@@ -7,4 +7,4 @@ var EE = new EventEmitter();

 assert.throws(function() {
  EE.emit('error', 'Accepts a string');
-}, /Accepts a string/);
+}, /Unhandled "error" event. \(Accepts a string\)/);


People should not be emitting strings or other things, even though we technically support it. So, in most cases, we'll print the rather unhelpful [object Object], like in your previous test, right?

Certainly in all of my team's code, we pass an object to the emitter so it would just spit that out, yes.

I'd opt to just not try to print the error. You could use util.format(), but it prints the whole stack trace for errors, so it's kind of loud.

Yeah, I just omitted it and made the test reflect a real world use case slightly better.

ctide · 2016-12-22T00:38:46Z

@Trott: I think maybe I was just on the wrong branch. It was my first time pulling down and running the node tests. I tried to recreate it to see if it was something with first time configuring & running tests, and it ran the linter.

lpinca · 2016-12-22T20:12:15Z

test/parallel/test-event-emitter-errors.js

@@ -7,4 +7,8 @@ var EE = new EventEmitter();

 assert.throws(function() {
  EE.emit('error', 'Accepts a string');
-}, /Accepts a string/);
+}, /Unhandled "error" event/);


Nit: how about using ^Error: Unhandled "error" event$? Pointing this out only because other tests do this.

italoacasas · 2016-12-23T01:37:09Z

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

joaocgreis · 2016-12-23T02:14:23Z

Re-run of arm-fanned because of the RPi1 failures: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6082/

jasnell

This LGTM but might have broader impact than expected. @nodejs/ctc thoughts?

targos · 2016-12-26T10:36:21Z

lib/events.js

@@ -160,7 +160,7 @@ EventEmitter.prototype.emit = function emit(type) {
      throw er; // Unhandled 'error' event
    } else {
      // At least give some kind of context to the user
-      var err = new Error('Uncaught, unspecified "error" event. (' + er + ')');


I'm not sure it's a good idea to remove the last part. I understand that if you emit a plain object, it will print [object Object] but it seems logical for the "error" event to emit an Error and in this case it would print the error message correctly.

yea, I would prefer that we keep the er in the message

thefourtheye · 2016-12-27T05:57:58Z

lib/events.js

@@ -160,7 +160,7 @@ EventEmitter.prototype.emit = function emit(type) {
      throw er; // Unhandled 'error' event
    } else {
      // At least give some kind of context to the user
-      var err = new Error('Uncaught, unspecified "error" event. (' + er + ')');
+      var err = new Error('Unhandled "error" event');


Nit: Prefer const over var wherever possible.

jasnell · 2017-03-22T22:11:49Z

@nodejs/ctc ... any further thoughts on this one?
@ctide ... if you'd like to pursue this can you please give it a quick rebase?

Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.

ctide · 2017-03-23T00:32:45Z

Rebased, and a few minor things based on the other feedback (adding er back, changing var to const, using fat arrow functions in the test instead.)

jasnell · 2017-03-23T17:30:18Z

@sam-github @cjihrig ... PTAL

jasnell · 2017-03-24T15:32:15Z

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

Update error message that's thrown when no error listeners are attached to an emitter. PR-URL: #10387 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

jasnell · 2017-03-24T23:11:32Z

Landed in 2141d37

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. lts-watch-v6.x labels Dec 21, 2016

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 21, 2016

mscdex reviewed Dec 21, 2016

View reviewed changes

mscdex removed the lts-watch-v6.x label Dec 21, 2016

ctide force-pushed the patch-1 branch from 1224188 to 262c325 Compare December 21, 2016 19:36