Give more context to unhandled error events#1654
Uh oh!
There was an error while loading. Please reload this page.
Conversation
evanlucas commented May 7, 2015
rvagg commented May 7, 2015
👍 I like this so lgtm |
cjihrig commented May 7, 2015
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? |
lib/events.js Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
so instead do something like so?:
er+=(newError()).stack.substr(6);There was a problem hiding this comment.
Wouldn't that make er a string though?
There was a problem hiding this comment.
Got this change and the one above it mixed up in context. nm what I said.
evanlucas commented May 7, 2015
@cjihrig That could work. I understand the concern about encouraging throwing strings. Sometimes, strings are thrown from dependencies. |
mscdex commented May 7, 2015
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 commented May 7, 2015
Ok, so maybe set whatever |
mscdex commented May 7, 2015
@evanlucas Yeah something like, I'm not sure what the best terminology would be...
|
evanlucas commented May 7, 2015
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? |
mscdex added the events cjihrig commented May 8, 2015
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 commented May 8, 2015
Ok, updated to append the argument to the default error message. The argument is also added to the error object as the |
cjihrig commented May 8, 2015
LGTM if everyone is OK with using |
evanlucas commented May 9, 2015
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 commented May 11, 2015
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
evanlucas commented May 12, 2015
Landed in 8b9a153 |
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
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
5 participants
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