Skip to content

Give more context to unhandled error events - #1654

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:events-error
May 12, 2015
Merged

Give more context to unhandled error events#1654
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:events-error

Conversation

@evanlucas

@evanlucasevanlucas commented May 7, 2015

Copy link
Copy Markdown
Contributor

Previously, in the event of an unhandled error event, if the error is a
not an actual Error, then a default error is thrown. Now, if a string
is the unhandled error, it is wrapped in an Error and then thrown.

This provides more context in the event a string is emitted for the
error event.

This PR also prints the error to stderr in the event it is not a string or an Error object.

I see this as being controversial, but it was quite minimal so I wanted to see what everyone else thinks.

/cc @iojs/collaborators

@evanlucasevanlucas changed the title Events errorGive more context to unhandled error eventsMay 7, 2015
@evanlucas

evanlucas commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

Ref: nodejs/node-v0.x-archive#14415

@rvagg

rvagg commented May 7, 2015

Copy link
Copy Markdown
Member

👍 I like this so lgtm

@cjihrig

cjihrig commented May 7, 2015

Copy link
Copy Markdown
Contributor

This almost encourages people to use strings as errors. I like reporting the data that was provided. What if you added it to end of the default error's message?

mscdex
mscdex reviewed May 7, 2015
View reviewed changes
Comment threadlib/events.js Outdated

@mscdexmscdexMay 7, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not so sure about this. Can't we just attach er to the thrown error object and/or concatenate the er to the thrown error message?

@trevnorristrevnorrisMay 7, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so instead do something like so?:

er+=(newError()).stack.substr(6);

@evanlucasevanlucasMay 8, 2015

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Wouldn't that make er a string though?

@trevnorristrevnorrisMay 8, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got this change and the one above it mixed up in context. nm what I said.

@evanlucas

evanlucas commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

@cjihrig That could work. I understand the concern about encouraging throwing strings. Sometimes, strings are thrown from dependencies.

@mscdex

mscdex commented May 7, 2015

Copy link
Copy Markdown
Contributor

With regards to string handling, I agree with @cjihrig. IMHO it's better to just submit PRs to those dependencies that are throwing strings.

@evanlucas

evanlucas commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

Ok, so maybe set whatever er is to something like er.context?

@mscdex

mscdex commented May 7, 2015

Copy link
Copy Markdown
Contributor

@evanlucas Yeah something like, I'm not sure what the best terminology would be...

  • .context
  • .unhandledError
  • .originalError/.origError
  • ??

@evanlucas

evanlucas commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

Ok, so we add a property to the error and if the argument is a string, append it to the error message?

And we want to not actually log the argument?

@mscdexmscdex added the events Issues and PRs related to the events subsystem / EventEmitter. label May 7, 2015
@cjihrig

cjihrig commented May 8, 2015

Copy link
Copy Markdown
Contributor

We should probably append the argument, no matter what the type. Seeing the bad value (with strings being the most likely case) might help with debugging.

@evanlucas

evanlucas commented May 8, 2015

Copy link
Copy Markdown
ContributorAuthor

Ok, updated to append the argument to the default error message. The argument is also added to the error object as the context property.

@cjihrig

cjihrig commented May 8, 2015

Copy link
Copy Markdown
Contributor

LGTM if everyone is OK with using context as the field name.

@evanlucas

evanlucas commented May 9, 2015

Copy link
Copy Markdown
ContributorAuthor

Thanks @cjihrig. I'll leave open for a few days to make sure everyone has a chance to voice his or her opinion. I will plan on merging on Tuesday if there are no objections.

@evanlucas

evanlucas commented May 11, 2015

Copy link
Copy Markdown
ContributorAuthor

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/658/

@evanlucas
events: provide better error message for unhandled error
8b9a153
Previously, in the event of an unhandled error event, if the error is a
not an actual Error, then a default error is thrown. Now, the argument
is appended to the error message and added as the `context` property
of the error.
PR-URL: nodejs#1654
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas
evanlucas deleted the events-error branch May 12, 2015 13:35
@evanlucas
evanlucas merged commit 8b9a153 into nodejs:masterMay 12, 2015
@evanlucas

evanlucas commented May 12, 2015

Copy link
Copy Markdown
ContributorAuthor

Landed in 8b9a153

@Fishrock123Fishrock123 mentioned this pull request May 13, 2015
Merged
Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 15, 2015
@Fishrock123
2015-05-15 io.js v2.0.2 Release
3e7a143
PR-URL: nodejs#1679
Notable Changes:
* win,node-gyp: the delay-load hook for windows addons has now been
correctly enabled by default, it had wrongly defaulted to off in the
release version of 2.0.0 (Bert Belder) nodejs#1433
* os: tmpdir()'s trailing slash stripping has been refined to fix an
issue when the temp directory is at '/'. Also considers which slash is
used by the operating system. (cjihrig) nodejs#1673
* tls: default ciphers have been updated to use gcm and aes128 (Mike
MacCana) nodejs#1660
* build: v8 snapshots have been re-enabled by default as suggested by
the v8 team, since prior security issues have been resolved. This
should give some perf improvements to both startup and vm context
creation. (Trevor Norris) nodejs#1663
* src: fixed preload modules not working when other flags were used
before --require (Yosuke Furukawa) nodejs#1694
* dgram: fixed send()'s callback not being asynchronous (Yosuke
Furukawa) nodejs#1313
* readline: emitKeys now keeps buffering data until it has enough to
parse. This fixes an issue with parsing split escapes. (Alex Kocharin)
* cluster: works now properly emit 'disconnect' to cluser.worker (Oleg
Elifantiev) nodejs#1386
events: uncaught errors now provide some context (Evan Lucas) nodejs#1654
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
@evanlucas@Fishrock123
events: provide better error message for unhandled error
69bc061
Previously, in the event of an unhandled error event, if the error is a
not an actual Error, then a default error is thrown. Now, the argument
is appended to the error message and added as the `context` property
of the error.
PR-URL: nodejs#1654
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@snyk-botsnyk-bot mentioned this pull request Mar 25, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

eventsIssues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@evanlucas@rvagg@cjihrig@mscdex@trevnorris