zwave-js override global Promise rejection mechanism which cause side-effects

[Pierre-Gilles]

Is your problem within Home Assistant (Core or Z-Wave JS Integration)?

NO, my problem is NOT within Home Assistant or the ZWave JS integration

Is your problem within ZWaveJS2MQTT?

NO, my problem is NOT within ZWaveJS2MQTT

Checklist

• I have checked the troubleshooting section and my problem is not described there.

• I have read the changelog and my problem was not mentioned there.

Describe the bug

Hi,

I'm the core maintainer of the open-source project Gladys Assistant, and we are trying to migrate to this library for our Z-Wave integration.

First, congrats on the work on this great lib 🙂

What causes the bug?

It seems that this library loads a third-party NPM module, @sentry/utils, a library that is not intended for production use and that override some Promise rejection behavior in the global scope.

It causes unHandledPromiseRejection to be handled differently as soon as we require the zwave-js module, and cause side-effects with other librairies.

Steps to reproduce the behavior

I managed to write a very simple script to reproduce the issue:

async function start() {
  throw new Error("error");
}

(async () => {
  // require("zwave-js");
  start();
})();

If you execute this script with the require commented, you'll get this result (in Node 14):

(node:14305) UnhandledPromiseRejectionWarning: Error: error
    at start (/Users/pierregilles/sandbox/zwavejs/index.js:2:9)
    at /Users/pierregilles/sandbox/zwavejs/index.js:7:3
    at Object.<anonymous> (/Users/pierregilles/sandbox/zwavejs/index.js:8:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:75:12)
    at internal/main/run_main_module.js:17:47
(Use `node --trace-warnings ...` to show where the warning was created)
(node:14305) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:14305) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

As soon as I uncomment the require, I get this result:

This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason:
Error: error
    at start (/Users/pierregilles/sandbox/zwavejs/index.js:2:9)
    at /Users/pierregilles/sandbox/zwavejs/index.js:7:3
    at Object.<anonymous> (/Users/pierregilles/sandbox/zwavejs/index.js:8:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:75:12)
    at internal/main/run_main_module.js:17:47

As you can see, the zwave-js library mutate a global object and cause errors to be thrown differently.

Which branches or versions?

version:
node-zwave-js branch: 8.11.6

[Pierre-Gilles]

After investigating a bit more, it seems this Sentry lib is setting up a global unhandledRejection handler:

https://github.com/getsentry/sentry-javascript/blob/4bbb553cc8f2ab6ba23302fc8e41c26ad90434c7/packages/node/src/integrations/onunhandledrejection.ts#L38

[AlCalzone]

Sentry is our error reporting, which needs to be able to catch unhandled rejections, otherwise that would defeat its purpose. I haven't figured out if (and how) its possible to limit Sentry to errors originating from zwave-js.

If necessary, I could add an env variable to disable initializing Sentry - how does that sound?

[AlCalzone]

Maybe disabling the unhandled rejection integration and instead using the uncaughtExceptionMonitor event would be an alternative. This didn't exist back when I added Sentry, but could be an option that doesn't interfere with Node.js as much.

[Pierre-Gilles]

Both solutions look great.

Maybe disabling entirely Sentry with an env variable is the easiest way to remove all possible side-effects (because I'm not sure this is the only one?), but the 2nd option works too!

[AlCalzone]

So further investigation reveals: uncaughtExceptionMonitor is only called when the --unhandled-rejections CLI option is strict or throw. Node.js < 15 has it on warn by default.

I think this is the way to go - capture errors explicitly instead of installing global "magic" handlers. Since the majority are on Node.js 16, we shouldn't miss out on too many actual errors this way.

Scratch that - doesn't work at all. I'm just going to make Sentry opt-in.

[AlCalzone]

This will be fixed in the v9 release

[Pierre-Gilles]

Thank you so much for having a look into it 🙏

[Pierre-Gilles]

@AlCalzone Just read your comment in the PR :

This can affect applications on Node.js before v15, which are going to crash instead of logging a warning. While we do believe no rejection should be silently unhandled, we don't want to break existing applications.

The Sentry listener has an impact on any version of Node (even Node 16), because the app can register a unhandledRejection listener to avoid having the application to crash (and log the issue somewhere), and Sentry would add a listener too which would force the crash even if the first listener was telling the app not to.

On embedded home automation software (alarm system), a hard crash (without the time to save data/make sure everything is fine) is really not wanted !

[AlCalzone]

You've got a point there. I'll change the sentry listener to behave like Node.js on "warn" level.

[rpochet]

FYI: getsentry/sentry-javascript#4154

[AlCalzone]

According to my tests, https://docs.sentry.io/platforms/node/configuration/integrations/default-integrations/#onunhandledrejection with the mode none does not exit the process, but captures the error and Node.js still logs a warning.