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

uncaughtException origin is always unhandledRejection when package.json type is module #41328

Closed
vladshcherbin opened this issue Dec 26, 2021 · 6 comments · Fixed by #41339
Closed
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@vladshcherbin
Copy link

Version

v16.13.1

Platform

mac os 10.14.6, Darwin uladzislaus-mbp 18.7.0 Darwin Kernel Version 18.7.0: Sun Dec 1 18:59:03 PST 2019; root:xnu-4903.278.19~1/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

  1. create a script file with contents:
process.on('uncaughtException', (_, origin) => {
  console.log('whoops, uncaughtException', origin)
})

unknownFunction()
  1. run node <name-of-the-file>
  2. you will see whoops, uncaughtException uncaughtException
  3. now, create a package.json file next to script file with contents:
{
  "type": "module"
}
  1. run node <name-of-the-file>
  2. now, the message will be whoops, uncaughtException unhandledRejection

How often does it reproduce? Is there a required condition?

In package.json file, type should be module to see the bug.

What is the expected behavior?

uncaughtException origin should be uncaughtException

What do you see instead?

uncaughtException origin is unhandledRejection

Additional information

No response

@Mesteery Mesteery added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 26, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 26, 2021

I suppose <name-of-the-file> has a .js extension, right? Setting "type": "module" makes node parse .js files as modules (ESM) rather than classic scripts. ESM parsing is asynchronous IIRC, which I guess explains where this unhandledRejection origin comes from.
Still, there's at least one thing that is wrong in the docs:

node/doc/api/process.md

Lines 336 to 340 in 86099a3

* `origin` {string} Indicates if the exception originates from an unhandled
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.

mkdir repro
cd repro
echo 'process.on("uncaughtException", (_, origin) => console.log(origin));unknownFunction()' > test.mjs
node --unhandled-rejections=none test.mjs # logs unhandleRejection, despite what the docs claim

@nodejs/modules is there a way to differentiate errors throw synchronously from unhandled rejections in an ES module? Or should we simply update the docs to document the current behavior?

@bmeck
Copy link
Member

bmeck commented Dec 26, 2021

I see this behavior as sadly expected/unavoidable but confusing like many ESM expectations broken from transpiled forms; we should update the docs. In certain cases we can detect if the error was thrown during evaluation but in general even with that, all handling is done through Promise style handling. E.G.

let nsPromise = import('a');
  • if a imports b and b throws the actual error isn't uncaught ever, it is always wrapped into a Promise when consumed via import().
  • if a uses top level await we cannot know synchronously if it threw / will be handled.
  • uncaughtException isn't thrown for other Promise like flows, even if using throw. E.G. prom.then(() => {throw new Error();}).

@vladshcherbin
Copy link
Author

Yes, the file extension is .js. I was reading uncaughtExceptionMonitor docs and wanted to use its origin to distinguish uncaughtException from unhandledRejection.

Hopefully, it can be fixed for at least some cases.
I believe this should also be mentioned in the docs for other devs as origin is very confusing in the case.

@bmeck
Copy link
Member

bmeck commented Dec 26, 2021

To be clear, I'm skeptical that we should change it without more understanding of the actual ask here. Right now the behavior of uncaughtException isn't reliable even in top level CJS contexts:

// note that this is synchronously executed:
new Promise(f => require('./file-that-throws.cjs'))

What is the exact thing we are trying to achieve rather than jumping to try and change behaviors? I see something about:

use its origin to distinguish uncaughtException from unhandledRejection.

Right now ESM never ever emits uncaughtExceptions and even we do determine when it should emit, it doesn't sound like it necessarily matches a given workflow since top level throw isn't even guaranteed to cause uncaughtException.

aduh95 added a commit to aduh95/node that referenced this issue Dec 27, 2021
aduh95 added a commit to aduh95/node that referenced this issue Dec 27, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 29, 2021

It was discussed in the linked PR, but I think it's worth pointing it out here: the title of this issue is not quite true, you can still get 'uncaughtException' as origin in ESM as long as the exception doesn't originate from a Promise-based context, e.g.:

process.on('uncaughtException', (_, origin) => {
  console.log('whoops, uncaughtException', origin)
})

setImmediate(() => unknownFunction())

@benjamingr
Copy link
Member

I think the whole goal of the unhandled rejection changes last year was for the behaviour of uncaughtException and unhandledRejection to be similar. It is very easy today to make an uncaughtException an unhandledRejection like Antoine and Bradley wrote.

nodejs-github-bot pushed a commit that referenced this issue Jan 9, 2022
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
targos pushed a commit that referenced this issue 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 issue 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 issue 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 issue Feb 1, 2022
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants