Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: show throw stack trace for uncaught exception #19003

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

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.


In its current form, this patch would not modify err.stack if 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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

events

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 26, 2018
lib/events.js Outdated
@@ -98,6 +98,58 @@ EventEmitter.prototype.getMaxListeners = function getMaxListeners() {
return $getMaxListeners(this);
};

// Returns the longest sequence of a that fully appears in b, of length >= 3.
Copy link
Contributor

@mscdex mscdex Feb 26, 2018

Choose a reason for hiding this comment

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

Perhaps use backticks around variables in these comments, otherwise 'a' looks like the article and not the variable and is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done.

@benjamingr
Copy link
Member

benjamingr commented Feb 26, 2018

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.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

I'm wondering if this has a measurable performance impact.

@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 error events without handlers is something that we should need to make as fast as possible.

I’ve also added more (but similar) tests, let me know if you had something specific in mind.

lib/events.js Outdated
}

function enhanceStackTrace(err, own) {
const sep = '\nEmitted \'error\' event at at:\n';
Copy link
Member

Choose a reason for hiding this comment

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

duplicate "at"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, good catch!

lib/events.js Outdated
}

const kExpandStack = Symbol('kExpandStack');
Object.defineProperty(EventEmitter, 'kExpandStack', { value: kExpandStack });
Copy link
Member

Choose a reason for hiding this comment

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

is that made public on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

Could it be exported by an internal module? Or is it too early in the bootstrapping process to do it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Could we add a test for process.on('uncaughtException', fn)? I guess right now that does not work together?

lib/events.js Outdated
return [ l1, j1, i1 ];
else
return [ l2, i2, j2 ];
}
Copy link
Member

Choose a reason for hiding this comment

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

This part confuses me. As far as I see it the common part will always be identical or do I miss something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@BridgeAR Thanks for catching – it made sense in an earlier version of this code, but it’s removed now.

addaleax added 5 commits March 1, 2018 22:33
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
Copy link
Member Author

addaleax commented Mar 1, 2018

I guess right now that does not work together?

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
Copy link
Member Author

addaleax commented Mar 2, 2018

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
@addaleax
Copy link
Member Author

addaleax commented Mar 4, 2018

Landed in 68d508a

@addaleax addaleax closed this Mar 4, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018
@addaleax addaleax deleted the events-stack branch March 4, 2018 21:27
addaleax added a commit that referenced this pull request Mar 4, 2018
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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Mar 5, 2018
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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants