-
Notifications
You must be signed in to change notification settings - Fork 30k
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
process.throwDeprecation
hides original error stack
#17871
Comments
This is normal behavior as the error is thrown async as most errors in Node.js. Tracking errors better can be done with async hooks on your side. Currently there is nothing that can be done to further improve this. Therefore I am closing this issue. |
When an error occurs in my code, I expect to see the place in my code where it occurred. I think this is a pretty reasonable expectation and this issue should stay open until it's met for the case I described. |
Refs #11865 @nylen there you will find more information about the problem. It has nothing to do with "your code". What is actually happening is something like function throwing() { throw new Error() }
function notIncludedInStack() { setTimeout(throwing, 1) }
notIncludedInStack() This will only show "throwing" in the stack trace and not the original function because the error is thrown async. |
Thanks for the context. Of course I'd like to have full async stack traces, but that's not really what this issue is about. In this specific case, I don't want to see any of these stack trace lines, because they're not relevant to the actual problem that occurred:
It's a bit frustrating to have this closed as "question" because all of the information needed to print a better stack trace is readily available. |
@nylen I will try to explain it with your code async function x() {
throw new Error( 'aaaa' );
} This translates into function x() {
return Promise.reject(new Error('aaaa'));
} As soon as the promise is rejected Node.js tracks if it was caught on the nextTick. This is not the case here and as soon as Node.js realizes this (remember, it is already in the next tick), a new Error is created. This error has a stack trace that has no knowledge about the function I hope this explains the problem better. |
We could defer the deprecation warning by a tick, that gives the UnhandledPromiseRejectionWarnings time to print. I.e., this diff: diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
index 82f15f9f9d..8985cd1a63 100644
--- a/lib/internal/process/promises.js
+++ b/lib/internal/process/promises.js
@@ -89,11 +89,18 @@ function setupPromises(scheduleMicrotasks) {
process.emitWarning(warning);
if (!deprecationWarned) {
deprecationWarned = true;
- process.emitWarning(
- 'Unhandled promise rejections are deprecated. In the future, ' +
- 'promise rejections that are not handled will terminate the ' +
- 'Node.js process with a non-zero exit code.',
- 'DeprecationWarning', 'DEP0018');
+ const warn = () => {
+ process.emitWarning(
+ 'Unhandled promise rejections are deprecated. In the future, ' +
+ 'promise rejections that are not handled will terminate the ' +
+ 'Node.js process with a non-zero exit code.',
+ 'DeprecationWarning', 'DEP0018');
+ };
+ if (process.throwDeprecation)
+ process.nextTick(warn); // Give warnings time to print.
+ else
+ warn();
}
}
Produces this output:
OTOH, a one-off hack like that is kind of yuck and it's not as if node isn't doing exactly what it's being told to - throw instead of emit. @nylen You can try opening a PR and see what others think. |
This is the root of the problem, I think. Why does Node have to create a new |
I agree with all of this. I think a better fix involves throwing the original error instead of |
Just wrote this in another script: // https://github.com/nodejs/node/issues/17871 :(
// process.throwDeprecation = true;
process.on( 'unhandledRejection', err => {
console.error( 'Unhandled promise rejection:', err );
process.exit( 1 );
} ); I want to like developing against Node.js with promises and async/await, but stuff like this doesn't make it easy. |
This is still broken, and I still don't understand why this issue was closed. |
I'm still writing this at the top of almost all my scripts: // https://github.com/nodejs/node/issues/17871 :(
process.on( 'unhandledRejection', err => {
console.error( 'Unhandled promise rejection:', err );
process.exit( 1 );
} ); |
This makes sure all warnings that were triggered before a deprecation warning during the same tick are properly handled and logged. It also guarantees that users can not catch the error anymore. Fixes: nodejs#17871
Node v9.3.0 on Linux (I would expect other versions since the introduction of the deprecation warning stuff - and other platforms - to behave the same way)
Current behavior
It doesn't matter whether you use
process.throwDeprecation = true
ornode --throw-deprecation
here, the behavior is the same.Expected behavior
Note concise output that preserves the original error stack without adding irrelevant stuff.
The text was updated successfully, but these errors were encountered: