-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
doc: clarify uncaughtException origin for ESM
#41339
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
Conversation
bmeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should workshop wording to make it clear that it isn't causes necessarily by ESM entrypoints but just having things wrapped in a Promise async context in general, see a purely CJS duplication:
// node --unhandled-rejections=strict entry.cjs
new Promise(() => require('./throws.cjs'));// throws.cjs
process.on('uncaughtExceptionMonitor', (...args) => {
console.log(args);
})
throw new Error('thrown');might mean just more clearly defining what an uncaughtException is since the terms are being used vaguely.
This comment has been minimized.
This comment has been minimized.
|
Hum it looks like you're correct, using the entry point is a red herring: mkdir repro
cd repro
echo 'process.on("uncaughtException", (_, origin) => console.log(origin));nonexistentFunc()' > test.cjs
node test.cjs # uncaughtException
node --experimental-specifier-resolution=node --unhandled-rejections=none test.cjs # unhandledRejection |
doc/api/process.md
Outdated
| [`--unhandled-rejections`][] flag set to `strict` or `throw` and | ||
| an unhandled rejection. | ||
| an unhandled rejection, or when a rejection happens during the ES module | ||
| static loading phase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just simplify this to when in a Promise based async context? Since purely with CJS and having no ESM this also happens in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a purely CJS example when this happen when --unhandled-rejections flag is NOT set to strict or throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduh95 I do not for that case, but it does happen outside of ES module static loading phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does happen outside of ES module static loading phase
Any chance we can list those edge cases as well? I don't think when in a Promise based async context does a great job at explaining when this will come up – my take away from #41328 is that it's not obvious to everyone that ESM loading is Promise based, so imho it's worth explicitly list it here rather than expect readers to know what is a Promise based context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a purely CJS example when this happen when
--unhandled-rejectionsflag is NOT set tostrictorthrow?
Didn’t the default for --unhandled-rejections change in 17? So it doesn’t need to be set anymore to get the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It changed in v15.0.0. But that's beside the point I think, this section of the docs is currently wrong, and if this PR doesn't do a good enough job to document when users should expect unhandledRejection rather than uncaughtException, I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduh95 roughly anything that creates a Promise job causes that, so IDK where to start. If an uncaughtException would occur inside of an async function / inside of a Promise constructor/handler callbacks / inside ESM are all the real times that this happens. So uncaughtException isn't really about the source file / throwing at the top level, but it is about how it was invoked, always. Listing async functions and Promise methods might be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async functions, promise then/catch/finally, new Promise executor functions, and soon, Array.fromAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything that creates a Promise job causes that, so IDK where to start
@bmeck I think that is true when --unhandled-rejections is set to strict or throw (which is default), and we're in agreement on this part. I'll try to rephrase the sentence so this part is clearer.
What is more tricky is that a rejection in the ES loader will always trigger an 'uncaughtException' event with origin 'unhandledRejection', even if --unhandled-rejections is set to none (or warn or warn-with-error-code). At least to me, this part is surprising, and in contradiction with what the docs are currently saying, so I think it's worth documenting it. I believe that the ES module loader is the only piece of node that behaves like this, but if you know of another example, I can include it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of another off the top of my head. I think we've done enough workshopping though that whatever is clarified seems good at this point.
|
Landed in 7edb50d |
|
Thanks for addition! As a non-native speaker I'll never understand what this means in the docs. |
|
@vladshcherbin would you like to send a PR with an improved wording? Or if you have a specific suggestion I can send the PR myself if you prefer. |
Fixes: #41328 PR-URL: #41339 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Fixes: nodejs#41328 PR-URL: nodejs#41339 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Fixes: nodejs#41328 PR-URL: nodejs#41339 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Fixes: #41328 PR-URL: #41339 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Fixes: #41328
/cc @nodejs/modules