events: show throw stack trace for uncaught exception#19003
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added events 
lib/events.js Outdated
There was a problem hiding this comment.
Perhaps use backticks around variables in these comments, otherwise 'a' looks like the article and not the variable and is confusing.
I'm +1 on both the design choices (Only modifying if thrown, and removing duplicate lines). I'm wondering if this has a measurable performance impact. |
benjamingr left a comment
benjamingr left a comment There was a problem hiding this comment.
Very nice! I'd love some more tests - this is a nice improvement. I wonder how common creating the error in a different context from throwing it happens.
addaleax commented Feb 26, 2018
@benjamingr I would expect that if you write a benchmark that tests for it, it will show an impact. I wouldn’t include such a benchmark in our suite though; I don’t think creating a ton of I’ve also added more (but similar) tests, let me know if you had something specific in mind. |
lib/events.js Outdated
lib/events.js Outdated
There was a problem hiding this comment.
@targos It’s non-enumerable, so most people will never find out it exists… I don’t think anybody is going to use it? How would we hide it?
There was a problem hiding this comment.
Could it be exported by an internal module? Or is it too early in the bootstrapping process to do it?
There was a problem hiding this comment.
@targos It’s fine, I’ve changes it – the worst case scenario is that this mechanism just doesn’t work and leaves the error unchanged.
BridgeAR left a comment
BridgeAR left a comment There was a problem hiding this comment.
Could we add a test for process.on('uncaughtException', fn)? I guess right now that does not work together?
lib/events.js Outdated
There was a problem hiding this comment.
This part confuses me. As far as I see it the common part will always be identical or do I miss something?
There was a problem hiding this comment.
@BridgeAR Thanks for catching – it made sense in an earlier version of this code, but it’s removed now.
Show the stack trace for the `eventemitter.emit('error')` call in the case of an uncaught exception. Previously, there would be no clue in Node’s output about where the actual `throw` comes from.addaleax commented Mar 1, 2018
It (intentionally) does not, yes, mostly to keep existing behavior unchanged & avoid this being semver-major. I don’t feel strongly about it, though. |
addaleax commented Mar 2, 2018
addaleax added the author ready addaleax commented Mar 4, 2018
Landed in 68d508a |
addaleax removed the author ready Show the stack trace for the `eventemitter.emit('error')` call in the case of an uncaught exception. Previously, there would be no clue in Node’s output about where the actual `throw` comes from. PR-URL: #19003 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>Show the stack trace for the `eventemitter.emit('error')` call in the case of an uncaught exception. Previously, there would be no clue in Node’s output about where the actual `throw` comes from. PR-URL: nodejs#19003 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Apr 13, 2018
Should this be backported to |
Show the stack trace for the `eventemitter.emit('error')` call in the case of an uncaught exception. Previously, there would be no clue in Node’s output about where the actual `throw` comes from. PR-URL: nodejs#19003 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>8 participants
Show the stack trace for the
eventemitter.emit('error')callin the case of an uncaught exception.
Previously, there would be no clue in Node’s output about where
the actual
throwcomes from.In its current form, this patch would not modify
err.stackif the exception is handled somewhere, and it redacts lines from the throw stack trace if they are duplicates of the original error stack trace (which likely occurs a lot).Those design choices are up for debate :)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
events