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.

Sign up for GitHub

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
Merged

doc: clarify uncaughtException origin for ESM #41339

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df5db76
doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 27, 2021
c3b2692
fixup! doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 27, 2021
61eb69a
fixup! doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 27, 2021
ffe2656
fixup! doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 27, 2021
c8d2454
fixup! doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 27, 2021
b589d69
fixup! doc: clarify `uncaughtException` `origin` for ESM
aduh95 Dec 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,9 @@ occurs. One of the following modes can be chosen:
set, trigger a warning, and set the process exit code to 1.
* `none`: Silence all warnings.

If a rejection happens during the ES module static loading phase, it will always
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
raise it as an uncaught exception.

### `--use-bundled-ca`, `--use-openssl-ca`

<!-- YAML
Expand Down
8 changes: 6 additions & 2 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


The `'uncaughtException'` event is emitted when an uncaught JavaScript
exception bubbles all the way back to the event loop. By default, Node.js
Expand Down Expand Up @@ -365,6 +366,8 @@ setTimeout(() => {
}, 500);

// Intentionally cause an exception, but don't catch it.
// Because the exception happens when evaluating an ES module, this is
// undistinguishable from a Promise rejection, and will be reported as such.
nonexistentFunc();
console.log('This will not run.');
```
Expand Down Expand Up @@ -433,7 +436,8 @@ added:
rejection or from synchronous errors. 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.

The `'uncaughtExceptionMonitor'` event is emitted before an
`'uncaughtException'` event is emitted or a hook installed via
Expand Down