Emscripten causes memory leaks when attaching events listeners to the `process`

[jeswr]

If I execute emcc helloworld.cpp -s MODULARIZE=1 -o dist/index.js on the following file

#include <stdio.h>

int main() {
  return 0;
}

then if I call the generated index.js module as follows

const helloWorld = require('./dist')

async function main() {
  for (let i = 0; i < 10_000; i += 1) {
    await helloWorld();
  }
}

each instantiation takes up an additional ~58KB of memory and the time taken to run each execution slowly decreases. For 10_000 calls it takes approximately 4min and 0.5GB of memory is consumed.

In contrast if I instantiate the Web-assembly file directly

const fs = require('fs');
const path = require('path');

async function main() {
  for (let i = 0; i < 10_000; i += 1) {
    const module = await WebAssembly.instantiate(fs.readFileSync(path.join(__dirname, 'dist', 'index.wasm')));
    await module.instance.exports.main()
  }
}

all 10_000 calls run in about 4 seconds and take up no memory (other than the 4MB of memory that the process had consumed before the first instantiation).

I have created a minimal reproduction here and this was first discovered as a problem here.

Version of emscripten/emsdk:
3.1.20

[jeswr]

This appears to be directly caused by emscripten attaching to the uncaughtException and unhandledRejection events. If I remove the following from the generated index.js files

  // MODULARIZE will export the module in the proper place outside, we don't need to export here

  process['on']('uncaughtException', function(ex) {
    // suppress ExitStatus exceptions from showing an error
    if (!(ex instanceof ExitStatus)) {
      throw ex;
    }
  });

  // Without this older versions of node (< v15) will log unhandled rejections
  // but return 0, which is not normally the desired behaviour.  This is
  // not be needed with node v15 and about because it is now the default
  // behaviour:
  // See https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode
  process['on']('unhandledRejection', function(reason) { throw reason; });

Then the problems seem to go away (that is, the results are the same as instantiating the main module directly)

So I would say this is directly related to #18659

[jeswr]

To me the ideal set of outcomes of this ticket would be:

• Don't attach unnecessary event listeners to the process on Node 16+
• Don't unecessarily attach listeners more than once Dont attach uncaughtException and unhandledRejection errors in modularise mode (c.f. Emscripten causes memory leaks when attaching events listeners to the process #18723 (comment))
• Remove the event listeners on the process once the run() call has finished
• Add a gating CI check which tests the memory consumption on the following environments; and error if the memory has increased by more than ~1MB over 10_000-100_000 calls:
  - All LTS Node versions.
  - Chrome and Firefox browser environments (on the main event loop, and in web workers).
• Possibly remove the uncaughtException listener entirely; since seems to just be suppressing errors of type ExitStatus on node.

I am willing to contribute to the effort of making these improvements.

[kripken]

@jeswr In general those goals sound good to me! Would be great to improve this.

For NODEJS_CATCH_REJECTION we do avoid that at compile time when possible,

https://github.com/emscripten-core/emscripten/blob/main/emcc.py#L2204-L2205

We could add the same for NODEJS_CATCH_EXIT. After that, once we update the minimal default node then this would not be a problem by default at least. I think that might be worth focusing on rather than improving the experience for older node, but if those improvements are simple I'm not opposed to them.

[sbc100]

@jeswr is the first item on your checklist taken care of by #18521?

[jeswr]

We could add the same for NODEJS_CATCH_EXIT.

That would be good :) Could you explain reason that this is needed in the first place and hence in what conditions it can be removed?

After that, once we update the minimal default node then this would not be a problem by default at least.

I assume this will happen in July when Node 14 reaches EOL?

@jeswr is the first item on your checklist taken care of by #18521?

Ah yes - though I just had a look at the code for it; getting the major version of node shouldn't require regex; I believe this line can be replaced with const nodeMajor = Number(process.versions.node.split(".")[0]).

[jeswr]

I've just tried to implement the "Don't unecessarily attach listeners more than once" point; this should look like

1. At the global scope having

#if NODEJS_CATCH_REJECTION
function throwReason(reason) {
  throw reason
}
#endif


#if NODEJS_CATCH_EXIT
function dontThrowExit(ex) {
  // suppress ExitStatus exceptions from showing an error
    if (!(ex instanceof ExitStatus)) {
      throw ex;
    }
}
#endif

2. Making

   emscripten/src/shell.js

   Lines 244 to 249 in e04f291

   	process['on']('uncaughtException', function(ex) {
   	// suppress ExitStatus exceptions from showing an error
   	if (!(ex instanceof ExitStatus)) {
   	throw ex;
   	}
   	});

if (!process.listeners('uncaughtException').includes(dontThrowExit)) {
  process['on']('uncaughtException', dontThrowExit);
}

3. And similarly making

   emscripten/src/shell.js

   Lines 259 to 261 in e04f291

   	if (nodeMajor < 15) {
   	process['on']('unhandledRejection', function(reason) { throw reason; });
   	}

if (nodeMajor < 15 && !process.listeners('unhandledRejection').includes(dontThrowExit)) {
    process['on']('unhandledRejection', dontThrowExit);
  }

I am struggling with the first part as I don't know the best place to be putting code so that it ends up in the global scope (even when MODULARISE is enabled). Any pointers with this?

[sbc100]

When MODULARIZE is enabled, there is no code emscripten-generated code in the global scope.. it all runs inside the module scope.

I think the solution here might simply be to default to NODEJS_CATCH_EXIT=0 and NODEJS_CATCH_REJECTION=0.. both of these options are really for running emscripten modules standalone, emulating command line tools. When you use MODULARIZE we can, at least by default, assume its up the embedding code to take care of these things.

[sbc100]

I think we could even say that NODEJS_CATCH_EXIT and NODEJS_CATCH_REJECTION are both incompatible with MODULARIZE.. for the reasons mentioned in this issue.