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

doc: clarify uncaughtException origin for ESM #41339

Merged
merged 6 commits into from
Jan 9, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 27, 2021

Fixes: #41328

/cc @nodejs/modules

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Dec 27, 2021
doc/api/process.md Outdated Show resolved Hide resolved
Copy link
Member

@bmeck bmeck left a 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.

@aduh95

This comment has been minimized.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 27, 2021

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/cli.md Outdated Show resolved Hide resolved
@@ -337,7 +337,8 @@ changes:
rejection or from an synchronous error. Can either be `'uncaughtException'` or
`'unhandledRejection'`. The latter is only used in conjunction with the
[`--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.
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Didn’t the default for --unhandled-rejections change in 17? So it doesn’t need to be set anymore to get the error.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 9, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 9, 2022
@nodejs-github-bot nodejs-github-bot merged commit 7edb50d into nodejs:master Jan 9, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7edb50d

@vladshcherbin
Copy link

Thanks for addition!

As a non-native speaker I'll never understand what this means in the docs.
I also do believe even for a native speaker it's hard to understand the use-case.

@aduh95 aduh95 deleted the esm-unhandleRejection branch January 9, 2022 12:20
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 9, 2022

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

targos pushed a commit that referenced this pull request Jan 14, 2022
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
Fixes: nodejs#41328

PR-URL: nodejs#41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Fixes: nodejs#41328

PR-URL: nodejs#41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uncaughtException origin is always unhandledRejection when package.json type is module
6 participants