Leaking `uncaughtException` and `unhandledRejection` handlers in node

[hoodmane]

If you start up many emscripten modules in an instance of node, each one adds the same uncaughtException handler and (in node <15) the same unhandledRejection warning. If you start up many modules it will cause a MaxListenersExceededWarning.

It would be better to use a global variable guard so that these are only added once. ex instanceof ExitStatus will only work for the one particular module because each ExitStatus class is distinct, for this changing the check to ex && ex.constructor && ex.constructor.name == "ExitStatus" would make one handler suffice for all Emscripten instances.

[sbc100]

Indeed this sounds like a annoying issue.

In order to run many emscripten modules I assume that you are also using something like MODULARIZE to encapsulate each one? This means that each module will define its own ExitStatus class and so it would likely not be sufficient to include just one handler.

Are you aware that both of these features can be disabled (using -sNODEJS_CATCH_EXIT=0 and -sNODEJS_CATCH_REJECTION=0)?

BTW are you using node < v15? I would imagine very few folks are.

[hoodmane]

you are also using something like MODULARIZE

Yes.

each module will define its own ExitStatus class and so it would likely not be sufficient to include just one handler.

I was suggesting ex && ex.constructor && ex.constructor.name == "ExitStatus" instead of instanceof ExitStatus to work around this.

are you using node < v15?

Well we still use node v14 for testing a lot since it is vendored with Emscripten. It also seems that we have some users that still use it, though supporting it is not a high priority for us since it's almost EOL.

[sbc100]

Well we still use node v14 for testing a lot since it is vendored with Emscripten.

Hopefully we will updating that version soon.

BTW, do you use the vendored version because you want to, or simply because emsdk puts in your PATH? i.e. would you rather emsdk didn't get this version to your PATH?

[sbc100]

Apologies for not reading your full bug report.. I see now your proposed solution.

What do you think about using -sNODEJS_CATCH_EXIT=0? Do you actually like that fact the emscripten installs this global handler? I would imagine this handler is mostly useful for things like unit test within emscripten itself, but perhaps its also useful in production node apps.

Another question, are you running emscripten in node in production, or just for testing purposes? (i.e. to avoid having to fire up the browser).

[hoodmane]

would you rather emsdk didn't get this version to your PATH?

Yes.

Are you aware that both of these features can be disabled (using -sNODEJS_CATCH_EXIT=0 and -sNODEJS_CATCH_REJECTION=0)?

Yes, I am going to start passing those. One thought: these global state changes are useful for when you are making an "application" but they are a bit weird when making a "library". Emscripten by default expects to be making an application. It might be nice to have a settings group like -sLIBRARY=1 that disables these sorts of global state changes by default. Maybe even having -sMODULARIZE disable these would be good since what I am really aiming for is "as much as possible do not modify global state". And if these settings are really only intended in "applications", they wouldn't need to be guarded.

[sbc100]

I like the idea of defaulting NODEJS_CATCH_REJECTION and NODEJS_CATCH_EXIT to off when MODULARIZE is used. And perhaps doing the same when no main function is exported?

[hoodmane]

And perhaps doing the same when no main function is exported?

Also maybe if INVOKE_RUN is false or noInitialRun is true.

are you running emscripten in node in production, or just for testing purposes?

Downstream users are using emscripten in node in production. I just use it for testing. Though the test systems also have downstream projects that use them to test their own stuff so even the testing libraries can feel a bit like production.

[jeswr]

I believe #18743 resolves this issue

[sbc100]

yes, I think between #18743 and #18521, this can now be closed

[hoodmane]

Thanks @sbc100 and @jeswr!