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

entire source of module printed when nodejs exception thrown #17228

Closed
dvtate opened this issue Jun 15, 2022 · 26 comments · Fixed by WebAssembly/binaryen#6764 or #22257
Closed

entire source of module printed when nodejs exception thrown #17228

dvtate opened this issue Jun 15, 2022 · 26 comments · Fixed by WebAssembly/binaryen#6764 or #22257

Comments

@dvtate
Copy link

dvtate commented Jun 15, 2022

This has been an issue that I've experienced with various packages built with emscripten since 2019 and honestly surprised it's still there. It's one of the biggest painpoints of working with emscripten + node.js for me personally

import binaryen from 'binaryen';

let s = "testing";

// Desparately trying to debug to figure out what problem is
// But this message gets burried in undesired garbage
console.log("s = ", s); 

// Fatal error which makes emscripten print massive unreadable textwall for no discernable reason
// filling console and making logs unreadable
s(444);

running the above code via node index.js results in the following message (which takes several seconds to print) and appears to be generated by emscripten
image

I've experienced this exact problem with every emscripten package I've used including internal ones at previous company so I don't think this is the fault of the package maintainers.

@kripken
Copy link
Member

kripken commented Jun 17, 2022

Trying to reproduce this, the following works:

  • In a new directory, make an empty package.json file with just { }.
  • npm install binaryen
  • Save the JS in the top comment as a.mjs - the mjs suffix is crucial.
  • node a.mjs in at least node 16.

I have no idea why node would print the contents of an imported package like that... the error isn't even in the same file! The stack trace even shows that. How bizarre... removing the module import line, the behavior is as expected. Just importing that module, not even using it, is enough to make node print it on an unrelated exception.

I'm not sure how to even debug that. Advice from any node experts reading this would be very welcome...

@kripken
Copy link
Member

kripken commented Jun 17, 2022

I can't reproduce this when generating the imported module. For example,

emcc tests/hello_world.c -s EXPORT_ES6 -o index.js -O3 --closure 1 -s SINGLE_FILE -s MODULARIZE

That emits an index.js that, when imported here, does not cause the bug. I tried lots of variations on those flags (which emit something in the same general shape as the binaryen build from npm).

If so then perhaps this is specific to binaryen.js somehow? cc @dcodeIO

@dvtate
Copy link
Author

dvtate commented Jun 17, 2022

Did you try calling the default export/making it actually load the wasm? I think it getting interrupted while instantiating the wasm module could be the problem. For whatever reason I'm getting an error with the __dirname variable (maybe Node v18 problem idk). But after hardcoding that to the correct value I was able to reproduce the issue with the hello_world.c test file.

image
image

 // required due to bug?
global.__dirname = '/tmp';

// /tmp/lib.mjs was generated via the following:
// emcc tests/hello_world.c -s EXPORT_ES6 -o /tmp/lib.mjs -O3 --closure 1 -s SINGLE_FILE -s MODULARIZE
import test from '/tmp/lib.mjs';

// Load module
console.log(test); // [Function (anonymous)]
test();

// Generate fatal error
let s = 'aaaaaa';
s(111111);

Versions and system info is as follows:

[tate@archbook emscr-bug]$ node --version
v18.3.0
[tate@archbook emscr-bug]$ emcc --version
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.9-git (54675bac246e84ca024c182e477665d21fe2e2f7)
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[tate@archbook emscr-bug]$ uname -a
Linux archbook 5.18.3-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 09 Jun 2022 16:14:10 +0000 x86_64 GNU/Linux

The problem stemming from loading wasm would make sense as the index.js file in binaryen.js ends with ,LB=qg;var tg=await LB(),rg=tg;export{rg as default}; which suggests it's loading a module in the import declaration. I tested a few other emscripten-compiled pacakages and thankfully most of them didn't suffer from this issue but wabt (only if you call it's default export) and talib-wasm (same way as binaryen; has a simple emcc call in build.sh).

It's honestly quite likely that this is a bug with node where it was loading things concurrently and highlights the line of code where both were at before the main thread crashed... I'm gonna have to disect the output js
image

@dvtate
Copy link
Author

dvtate commented Jun 17, 2022

the deeper I go the weirder this gets... still, I'm pretty sure this is a nodejs problem
image

@dcodeIO
Copy link
Contributor

dcodeIO commented Jun 17, 2022

I have also seen this issue when using Binaryen.js specifically. What appears to happen is that recent versions of Node.js try to print the source code location of where the Error was generated, in turn printing the entire minified JS produced by Emscripten - I guess because it's all a single line. As a workaround, I have found that the issue can be mitigated by running Node.js with --enable-source-maps, which instead prints a proper code location instead of a wall of text.

@kripken
Copy link
Member

kripken commented Jun 17, 2022

Interesting. Ok, given that, it would be great if we could make as small a testcase as possible and file a node.js issue with it for them to look at. If we can't reduce it, all of binaryen.js might be ok too as the bug seems really obvious.

@kleisauke
Copy link
Collaborator

This is probably a duplicate of #11532 and #14270, could you try to add -sNODEJS_CATCH_EXIT=0 -sNODEJS_CATCH_REJECTION=0 to the linker invocation? Note that this requires at least Node v15 to work properly.

emscripten/src/settings.js

Lines 733 to 751 in 854adf6

// By default we handle exit() in node, by catching the Exit exception. However,
// this means we catch all process exceptions. If you disable this, then we no
// longer do that, and exceptions work normally, which can be useful for libraries
// or programs that don't need exit() to work.
// Emscripten uses an ExitStatus exception to halt when exit() is called.
// With this option, we prevent that from showing up as an unhandled
// exception.
// [link]
var NODEJS_CATCH_EXIT = true;
// Catch unhandled rejections in node. Without this, node may print the error,
// and that this behavior will change in future node, wait a few seconds, and
// then exit with 0 (which hides the error if you don't read the log). With
// this, we catch any unhandled rejection and throw an actual error, which will
// make the process exit immediately with a non-0 return code.
// This should be fixed in Node 15+.
// [link]
var NODEJS_CATCH_REJECTION = true;

@matpen
Copy link

matpen commented Mar 19, 2023

I had the same problem using embind 3.1.23 on NodeJs 18.12: the solution suggested above works for me.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 19, 2023

I confirmed that this is no longer and issue after #18743 since we no longer enable NODEJS_CATCH_EXIT in MODULARIZE mode. This change was released in 3.1.34.

@decodio, I'm a little confused by the fact that you say this problem was mitigated by passing --enable-source-maps. I think perhaps that is really happening is that passing -gsource-map to emcc at link time disables the minification. So I don't think its the node flag that makes a difference here. For one thing, when we minify the JS library code I don't think we generate a source map for that. IIRC our source map support only applies to the native code, no the JS code. So I think maybe --enable-source-maps is a red herring here and that your output JS was not actually minified at all?

Regardless, I think this issue can now be closed. For folks that don't use -sMODULARZE, you can work around this issue by passing -sNODEJS_CATCH_EXIT=0 explicitly.

@dcodeIO
Copy link
Contributor

dcodeIO commented Mar 19, 2023

I can tell for certain that the Node.js flag makes a difference, but I also don't know why exactly. Perhaps it is merely a side-effect of making Node look for a source map in the first place, then suppressing the longish source output, or it only applies to cases where the application that imports Binaryen utilizes source maps itself. Happy if there is a better way, of course :)

@sbc100
Copy link
Collaborator

sbc100 commented Mar 19, 2023

I think this can now be closed

@sbc100 sbc100 closed this as completed Mar 19, 2023
@lastmjs
Copy link

lastmjs commented Jul 16, 2024

Hi everyone, I am really struggling with this issue. I am not using emscripten directly, but I am using binaryen.js with tsx (instead of ts-node) and Node.js v20.11.0.

This issue keeps happening and I've forked tsx to try to get to the bottom of it, but it seems like this is a Node.js issue.

In fact I just tested this without tsx, in Node.js directly, and it's still a problem. --enable-source-maps makes no difference. It's extremely simple to reproduce:

Here's my code index.js:

import "binaryen";

throw new Error("Should not see all of the code above");

package.json:

{
  "type": "module",
  "dependencies": {
    "binaryen": "^116.0.0"
  }
}

Run node index.js or node --enable-source-maps index.js and you will see an absolutely huge wall of text before the final error, I've only included the end of the wall of text below:

String("utf-8"):g=null}else typeof window!="undefined"&&typeof window.prompt=="function"?(g=window.prompt("Input: "),g!==null&&(g+=`
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            

Error: Should not see all of the code above
    at file:///home/lastmjs/development/test/test_tsx/index.js:3:7

Node.js v20.11.0

@sbc100
Copy link
Collaborator

sbc100 commented Jul 16, 2024

Presumably this is because the published version of binaryen.js is minified JS, and we don't produce any kind of source map when we do that minification. Is it normal for folks to provide source maps when then ship minified JS in npm?

I'm not very familiar with npm best practices but perhaps the JavaScript in npm itself should not be minified and it should be up to users/blunders of binaryen.js to minify the JS? Is it normal to ship minified JS in npm?

@kripken
Copy link
Member

kripken commented Jul 17, 2024

Hmm, I can't reproduce this when I build locally. But I do see it with the npm version.

So as @sbc100 said above,

I confirmed that this is no longer and issue after #18743 since we no longer enable NODEJS_CATCH_EXIT in MODULARIZE mode. This change was released in 3.1.34.

That should have fixed it. And it looks fixed when I build locally in the normal way (as Binaryen does on CI):

$ emcmake cmake -B emcc-build -DCMAKE_BUILD_TYPE=Release -G Ninja
$ ninja -C emcc-build binaryen_wasm

The resulting file is 1.65% smaller than the npm build, and it doesn't have the problem.

To make sure I am comparing them in the most similar way, I copied the build's JS over the index.js of the npm build in its directory (so I am not importing them in a different manner or anything like that).

@dcodeIO is the npm build made with different flags?

@CountBleck
Copy link

@kripken the npm version runs esbuild to bundle a small wrapper file that instantiates and re-exports Binaryen using top-level-await.

@kripken
Copy link
Member

kripken commented Jul 17, 2024

Thanks @CountBleck, that explains what I was seeing - without that await, my version wasn't actually being run.

Now that I have that working, I do see that some of the binaryen.js code is quoted in the stack trace even in my build. I see actually a lot less code quoted, because the minified npm build has 9 newlines but the raw emcc optimized output has 477, so it is slightly less minified. (I believe that is intentional in Emscripten, we leave newlines as separators where possible instead of ;, for this exact reason, and I guess whatever bundler is used here does not.) But, anyhow, that is a minor issue.

I then confirmed @kleisauke 's suggestion that those two flags -sNODEJS_CATCH_EXIT=0 -sNODEJS_CATCH_REJECTION=0 fix this. As binaryen.js is meant to be used as a library I think that's the right default here, I'll open a PR for that.

@sbc100 you say those flags should not be needed in MODULARIZE mode since some point in time, but we don't seem to use that flag in binaryen.js - I suppose the modularization is done by the bundler or something else, so those flags are needed.

kripken added a commit to WebAssembly/binaryen that referenced this issue Jul 17, 2024
…#6764)

Fixes emscripten-core/emscripten#17228

This seems the right default in binaryen.js which is used as a library through
npm mostly. We aren't a main program that wants to control node exclusively.
@kripken
Copy link
Member

kripken commented Jul 17, 2024

With WebAssembly/binaryen#6764 landed, this should be fixed in the next build of binaryen.js.

@CountBleck
Copy link

Hmm, is this correct Node.js behavior though? NODEJS_CATCH_EXIT only causes errors to be rethrown, and that shouldn't cause the printed line to change...

sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that there are very few test cases where it helps to
enable this settings, so disabling by default makes sense.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
@sbc100
Copy link
Collaborator

sbc100 commented Jul 18, 2024

Hmm, is this correct Node.js behavior though? NODEJS_CATCH_EXIT only causes errors to be rethrown, and that shouldn't cause the printed line to change...

While the stacktrace does remain intact on rethrow (only you throw something that is subclass of Error though), the line that gets printed before the stacktrace is thrown seems to be the rethrow line, and not the original line:

$ cat test.js
function foo() {
  console.log('foo');
  throw Error("bar");
}

process.on('uncaughtException', (ex) => {
  console.log('rethrowing');
  throw ex
});


foo();
$ node test.js 
foo
rethrowing
/usr/local/google/home/sbc/dev/wasm/emscripten/test.js:8
  throw ex
  ^

Error: bar
    at foo (/usr/local/google/home/sbc/dev/wasm/emscripten/test.js:3:9)
    at Object.<anonymous> (/usr/local/google/home/sbc/dev/wasm/emscripten/test.js:12:1)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
    at Module.load (node:internal/modules/cjs/loader:1203:32)
    at Module._load (node:internal/modules/cjs/loader:1019:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:128:12)
    at node:internal/main/run_main_module:28:49

Node.js v18.20.3

Note that line 8 of the file (the rethrow) is printed and not line 3 (the original throw).

If you throw something that isn't an Error subclass all traces of the original throw location seem to be lost in a rethrow.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 18, 2024

I'm uploaded a PR to disable this catching by default: #22257

We could followup by completely removing it I think.

@CountBleck
Copy link

@sbc100 yeah, I'm observing the same thing.

sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit to sbc100/emscripten that referenced this issue Jul 18, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228
sbc100 added a commit that referenced this issue Aug 4, 2024
It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: #17228
@chrispahm
Copy link

I'm still facing this issue despite setting NODEJS_CATCH_EXIT=0 -s NODEJS_CATCH_REJECTION=0. Using emcc v3.1.72 and Node.js v22.11.0. I'm building my module (the GEOS library) using

EMCC_CORES=4 
CFLAGS="-O3 -fexceptions" 
emcc ./native/usr/lib/libgeos.a ./native/usr/lib/libgeos_c.a -o ./package/geos.js\
  -O3 --closure 1 -s SINGLE_FILE=1 -s ALLOW_TABLE_GROWTH=1 -s ERROR_ON_UNDEFINED_SYMBOLS=1\
  -s FORCE_FILESYSTEM=0 -s ALLOW_MEMORY_GROWTH=1 -s DISABLE_EXCEPTION_CATCHING=0\
  -s NODEJS_CATCH_EXIT=0 -s NODEJS_CATCH_REJECTION=0 -s WASM=1 -s EXPORT_ES6=1\
  -s MODULARIZE=1 -s 'EXPORT_NAME="CModule"'\
  -s EXPORTED_FUNCTIONS="[ '... exported functions ...' ]"\
  -s EXPORTED_RUNTIME_METHODS="[ 'addFunction', 'removeFunction', 'setValue', 'getValue', 'ccall', 'cwrap', 'UTF8ToString', 'stringToUTF8' ]"

Link to repo with install instructions for reproducability

When an error is thrown, I get the correct error and stack trace, yet the entire module is printed before the error:
image

Building with -s DISABLE_EXCEPTION_CATCHING=1 still produces the wall of text, but without a useful error message shown in the actual error message.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 25, 2024

I believe this is simply due to the fact that we minify the code so that its all on a single line. Assume that its node itself that is printing this exception (is that correct?) then I'm not sure there is much you can do other than maybe not minifying? You can disable minification with --minify=0.

If the exception is being printed by emscripten code itself its possible we could do something, but I think this exception is escaping to the top level, right?

@chrispahm
Copy link

Thanks for your reply @sbc100! I've been checking again and you're right, node tries to print the line that threw the error, which is part of my library but outside of the binaryen code itself. Since the library is eventually minified to a single line using @rollup/terser still the entire source code is being printed. So in fact nothing that could be changed on the emscripten side!

Looking through the terser docs, I found the format.semicolons option that replaces ; with newlines wherever possible.

// ... rollup.config.js
plugins: [
  terser({
    format: {
      semicolons: false
    }
  })
]

With the additional newlines the wall of text vanishes when an uncaught exception occurs!
image

As the docs state, the only downside is that the gzipped bundle sizes might increase insignificantly though. In my case it grew from 782 KB to 783 KB (2.6 MB uncompressed), so indeed insignificantly. Definitely worth not having the console spammed with the entire source code! Leaving this here in case someone stumbles into the same issue.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 25, 2024

In emscripten we use the beautify: true terser setting to achieve this (although it does look like its been deprecated it still seems to work). That is how --minify=0 works on our end.

@chrispahm
Copy link

chrispahm commented Nov 25, 2024

I see, thanks for the explanation! I was expecting the beautify + --minify=0 setting to increase bundle sizes more than just the semicolon option, but it seems it's not making much of a difference in my case. Gzipped bundle is 785 KB with beautify + --minify=0, so just 2 KB more than semicolon + --minify=1. Again, thanks for the help!

chrispahm added a commit to chrispahm/geos-wasm that referenced this issue Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants