promises: improve unhandledrejection warnings #12982
Conversation
Use a linked list instead of an array for tracking unhandled rejections. Using shift and push have had some perf limitations in V8 lately. Using the linked list approach avoids that issue and ensures constant time adds and shifts.
* Set the warning.detail to reason.stack so the rejection stack is always printed (preserve the original warning.stack) * Set the warning.reason property to the rejection reason
|
I don't think this is in anyway a replacement for my PRs, but ti seems like a tangentially good idea to expose the Promise's actual stack rather than the internals of where the error was "caught" at. I'd rather just replace the current stack as exposed to |
|
The first 2 commits in here seem okay but not really related to the stack trace change itself? Fwiw all that I had in mind when talking about this was basically turning |
|
Changing the |
|
@jasnell can you add a test with two promise rejected? From what I expect, the stacktrace of the original error should get printed twice. |
|
This is semver-major as it changes the output. |
mcollina
added
semver-major
|
How would I use this in userland to crash if I have an unhandled rejection? I think we should cover this in a unit test maybe? |
|
Will be returning to this PR this week. Hopeful to get it landed by end of week. |
| let lastPromiseId = 1; | ||
|
|
||
| exports.setup = setupPromises; | ||
| class RejectedList { |
There was a problem hiding this comment.
Why not simply class RejectedList extends Array { push(promise, reason) {super.push({promise, reason})} }?
|
Closing this due to lack of progress. |
Per the CTC discussion around #12734 ...
This takes an alternative approach than #12734. Rather than adding a throw, it improves the warnings that are already there such that the stack trace for the rejection reason is always shown (assuming the reason is an Error object). It also adds a
warning.reasonproperty to the warning object emitted byprocess.on('warning'), allowing userland code to access the original error (and potentially throw if it wishes to do so).other code cleanups were made while I was in there.
/cc @nodejs/ctc
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
promises