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

Why catch unhandled rejections? #7855

Closed
arcanis opened this issue Jan 13, 2019 · 5 comments
Closed

Why catch unhandled rejections? #7855

arcanis opened this issue Jan 13, 2019 · 5 comments
Assignees
Labels

Comments

@arcanis
Copy link

arcanis commented Jan 13, 2019

This is a problem I always eventually have when working with Emscripten, so I figured I should open an issue because at the moment it makes for painful debugging sessions and error reports if you forget this detail.

Why is Emscripten's default shell configured to automatically catch unhandled rejections and exit the program? Shouldn't that be a decision taken by the user of the Emscriptened code, rather than Emscripten itself? This is even more problematic when Emscripten is used to compile a library, because then it silently bleeds into the code of the consumer applications - and goes unnoticed until it actually matters.

In particular, this problem causes stacktraces to disappear and to be replaced by an unhelpful generic error message (since Node doesn't print the exception if it sees nothing bound to unhandledRejection).

cc @dschuff @kripken
ref #5948 / #7135

@kripken kripken self-assigned this Jan 14, 2019
@kripken
Copy link
Member

kripken commented Jan 15, 2019

Yeah, I'd love to remove this too. Running our test suite to remember why we have it:

  • on node 8.12 an unhandled rejection at hello world startup (say, invalid wasm binary contents due to them being corrupted somehow) prints out the error, then a warning about an unhandled rejection, a deprecation notice, then hangs for a few seconds, then exits with returncode 0.
  • node node 10.15, the error is logged, it hangs, then it prints the warning and deprecation etc, then exits with returncode 0.

So in both cases an error is handled very awkwardly, and worst no error code is emitted, so tests might miss stuff (which happens on the wasm waterfall, 2 unexpected successes).

On the other hand, I don't understand the issue you're seeing, since I do see stack traces emitted, while you say they disappear. Do you have a testcase I can see that with?

@kripken
Copy link
Member

kripken commented Jan 15, 2019

(and in the emscripten test suite, other.test_only_force_stdlibs fails also for an unexpected success)

@roccomuso
Copy link
Contributor

is there a flag to remove it?

kripken added a commit that referenced this issue Jul 22, 2019
…uring async instantiation may behave oddly (they hang for a few seconds, node warns this behavior will be fixed in the future, and it exits with a 0 returncode), which is a downside, but the upside is that it avoids confusion for developers that get the handler popping up when they didn't expect it. Alter a test to work around this. Fixes #7855, #9028.
kripken added a commit that referenced this issue Jul 22, 2019
…errors during async instantiation may behave oddly (they hang for a few seconds, node warns this behavior will be fixed in the future, and it exits with a 0 returncode), which is a downside, but the upside is that it avoids confusion for developers that get the handler popping up when they didn't expect it. Alter a test to work around this. Fixes #7855, #9028."

This reverts commit f132637.
@kripken
Copy link
Member

kripken commented Jul 23, 2019

#9028 is open to add a flag for this.

kripken added a commit that referenced this issue Jul 26, 2019
…jections in node (#9061)

* Add NODEJS_CATCH_REJECTION, a flag that allows disabling the catching of unhandled rejections in node. This is useful if a dev doesn't want it to interfere with their own code. We don't set this to 0 by default because it would mean some wasm errors end up not returning a non-zero return code from node (still true as of 12.4.0)

Helps #7855, #9028
rlidwka added a commit to lvgl/lv_font_conv that referenced this issue Oct 24, 2019
rlidwka added a commit to lvgl/lv_font_conv that referenced this issue Oct 24, 2019
puzrin pushed a commit to lvgl/lv_font_conv that referenced this issue Oct 24, 2019
@stale
Copy link

stale bot commented Jul 22, 2020

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jul 22, 2020
@stale stale bot closed this as completed Aug 22, 2020
belraquib pushed a commit to belraquib/emscripten that referenced this issue Dec 23, 2020
…jections in node (emscripten-core#9061)

* Add NODEJS_CATCH_REJECTION, a flag that allows disabling the catching of unhandled rejections in node. This is useful if a dev doesn't want it to interfere with their own code. We don't set this to 0 by default because it would mean some wasm errors end up not returning a non-zero return code from node (still true as of 12.4.0)

Helps emscripten-core#7855, emscripten-core#9028
andersk added a commit to andersk/ttf2woff2 that referenced this issue Mar 5, 2021
When using ttf2woff2 as a library (e.g. inside webfonts-loader), this
was previously catching promise rejections from other code that had
nothing to do with ttf2woff2.

See emscripten-core/emscripten#7855.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/ttf2woff2 that referenced this issue Mar 5, 2021
When using ttf2woff2 as a library (e.g. inside webfonts-loader), this
was previously catching promise rejections from other code that had
nothing to do with ttf2woff2.

See emscripten-core/emscripten#7855.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
robhogan added a commit to robhogan/hermes that referenced this issue Mar 13, 2022
Summary:
Currently, initialising `emhermesc.js` (eg by requiring `metro-hermes-compiler`, or just `metro`) sets an uncaught exception handler on the global NodeJS host process:

```
process.on(
  'uncaughtException',
  function (ex) {if (!(ex instanceof ExitStatus)) {throw ex;}},
);
```

And similarly, an `unhandledRejection` handler
```
process["on"]("unhandledRejection", function (reason) {
  throw reason;
});
```

Because these are persistent, global handlers, they affect the behaviour of the host application or other libraries at any time after `emhermesc.js` is initialised - even if it's never used.

As far as NodeJS is concerned, `uncaughtException` listeners should never throw. Node prints the stack trace starting with the entirety of the emhermesc source, and then exits with [code 7 (Internal exception handler runtime failure)](https://nodejs.org/api/process.html#exit-codes). In cases where this output is truncated (eg, CI), the cause of the original exception is lost.

The `unhandledRejection` listener is [apparently](emscripten-core/emscripten#7855 (comment)) to avoid "awkward" output in old (<=10) Node versions. It [serves no purpose at all since Node 15](https://developer.ibm.com/blogs/nodejs-15-release-blog/), but still messes up stack traces when the `throw` comes from a huge single line of minified code. It's particularly unnecessary in `emhermesc`'s case since I don't think we expose any async API.

The injection of this code is gated in the emscripten build step behind `#ifdef NODEJS_CATCH_EXIT` and `#ifdef NODEJS_CATCH_REJECTION` respectively.

Context from emscripten
 - [Handler addition code in `shell.js`](https://github.com/emscripten-core/emscripten/blob/main/src/shell.js#L217-L233)
 - [Issue: Code generated by emscripten is catching all uncaught exceptions](emscripten-core/emscripten#5957)
 - [Issue: Why catch unhandled rejections?](emscripten-core/emscripten#7855)
 - [Docs for the flags](https://github.com/emscripten-core/emscripten/blob/main/src/settings.js#L704-L722)

Reviewed By: neildhar

Differential Revision: D34790221

fbshipit-source-id: e8b355e07022f4c377d918aa7dfdefe3d5272639
robhogan added a commit to robhogan/hermes that referenced this issue Mar 16, 2022
…N=0 (facebook#698)

Summary:
Pull Request resolved: facebook#698

Currently, initialising `emhermesc.js` (eg by requiring `metro-hermes-compiler`, or just `metro`) sets an uncaught exception handler on the global NodeJS host process:

```
process.on(
  'uncaughtException',
  function (ex) {if (!(ex instanceof ExitStatus)) {throw ex;}},
);
```

And similarly, an `unhandledRejection` handler
```
process["on"]("unhandledRejection", function (reason) {
  throw reason;
});
```

Because these are persistent, global handlers, they affect the behaviour of the host application or other libraries at any time after `emhermesc.js` is initialised - even if it's never used.

As far as NodeJS is concerned, `uncaughtException` listeners should never throw. Node prints the stack trace starting with the entirety of the emhermesc source, and then exits with [code 7 (Internal exception handler runtime failure)](https://nodejs.org/api/process.html#exit-codes). In cases where this output is truncated (eg, CI), the cause of the original exception is lost.

The `unhandledRejection` listener is [apparently](emscripten-core/emscripten#7855 (comment)) to avoid "awkward" output in old (<=10) Node versions. It [serves no purpose at all since Node 15](https://developer.ibm.com/blogs/nodejs-15-release-blog/), but still messes up stack traces when the `throw` comes from a huge single line of minified code. It's particularly unnecessary in `emhermesc`'s case since I don't think we expose any async API.

The injection of this code is gated in the emscripten build step behind `#ifdef NODEJS_CATCH_EXIT` and `#ifdef NODEJS_CATCH_REJECTION` respectively.

Context from emscripten
 - [Handler addition code in `shell.js`](https://github.com/emscripten-core/emscripten/blob/main/src/shell.js#L217-L233)
 - [Issue: Code generated by emscripten is catching all uncaught exceptions](emscripten-core/emscripten#5957)
 - [Issue: Why catch unhandled rejections?](emscripten-core/emscripten#7855)
 - [Docs for the flags](https://github.com/emscripten-core/emscripten/blob/main/src/settings.js#L704-L722)

Reviewed By: neildhar

Differential Revision: D34790221

fbshipit-source-id: 19782de6af0d4e26a91746443abb528a239932a0
facebook-github-bot pushed a commit to facebook/hermes that referenced this issue Mar 16, 2022
…N=0 (#698)

Summary:
Pull Request resolved: #698

Currently, initialising `emhermesc.js` (eg by requiring `metro-hermes-compiler`, or just `metro`) sets an uncaught exception handler on the global NodeJS host process:

```
process.on(
  'uncaughtException',
  function (ex) {if (!(ex instanceof ExitStatus)) {throw ex;}},
);
```

And similarly, an `unhandledRejection` handler
```
process["on"]("unhandledRejection", function (reason) {
  throw reason;
});
```

Because these are persistent, global handlers, they affect the behaviour of the host application or other libraries at any time after `emhermesc.js` is initialised - even if it's never used.

As far as NodeJS is concerned, `uncaughtException` listeners should never throw. Node prints the stack trace starting with the entirety of the emhermesc source, and then exits with [code 7 (Internal exception handler runtime failure)](https://nodejs.org/api/process.html#exit-codes). In cases where this output is truncated (eg, CI), the cause of the original exception is lost.

The `unhandledRejection` listener is [apparently](emscripten-core/emscripten#7855 (comment)) to avoid "awkward" output in old (<=10) Node versions. It [serves no purpose at all since Node 15](https://developer.ibm.com/blogs/nodejs-15-release-blog/), but still messes up stack traces when the `throw` comes from a huge single line of minified code. It's particularly unnecessary in `emhermesc`'s case since I don't think we expose any async API.

The injection of this code is gated in the emscripten build step behind `#ifdef NODEJS_CATCH_EXIT` and `#ifdef NODEJS_CATCH_REJECTION` respectively.

Context from emscripten
 - [Handler addition code in `shell.js`](https://github.com/emscripten-core/emscripten/blob/main/src/shell.js#L217-L233)
 - [Issue: Code generated by emscripten is catching all uncaught exceptions](emscripten-core/emscripten#5957)
 - [Issue: Why catch unhandled rejections?](emscripten-core/emscripten#7855)
 - [Docs for the flags](https://github.com/emscripten-core/emscripten/blob/main/src/settings.js#L704-L722)

Reviewed By: neildhar

Differential Revision: D34790221

fbshipit-source-id: 645e514db4107127b849e3a0baae088bd1ed9b50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants