From d48b48fd8bec207772c318de231f0be5eb33969f Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Wed, 17 Jul 2024 16:54:03 -0700 Subject: [PATCH] Disable NODEJS_CATCH_EXIT by default. NFC 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 --- .circleci/config.yml | 8 +++--- ChangeLog.md | 3 +++ .../tools_reference/settings_reference.rst | 2 +- src/library_browser.js | 14 +++++----- src/settings.js | 2 +- test/common.py | 7 ++--- test/test_core.py | 2 +- test/test_fs_after_main.c | 6 +++-- test/test_other.py | 27 ++++++++++--------- 9 files changed, 38 insertions(+), 33 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a13573111bfa2..f69c7dcbe870a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -320,10 +320,10 @@ commands: environment: HOMEBREW_NO_AUTO_UPDATE: "1" command: | - brew list cmake || brew install cmake - brew list ninja || brew install ninja - brew list python3 || brew install python3 - brew list pkg-config || brew install pkg-config + brew list pkg-config > /dev/null || brew install pkg-config + brew list cmake > /dev/null || brew install cmake + brew list ninja > /dev/null || brew install ninja + brew list python3 > /dev/null || brew install python3 - checkout - run: name: submodule update diff --git a/ChangeLog.md b/ChangeLog.md index 12c5aa282acef..530fb352a971e 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -20,6 +20,9 @@ See docs/process.md for more on how version tagging works. 3.1.64 (in development) ----------------------- +- The `NODEJS_CATCH_EXIT` setting is now disabled by default. This setting + is only useful under very specific circumstances, and has some downsides, so + disabling it by default makes sense. - Updated the SCons tool to not require the `EMSCRIPTEN_ROOT` environment variable, in which case it will assume that SCons will find the binaries in (its) `PATH`. diff --git a/site/source/docs/tools_reference/settings_reference.rst b/site/source/docs/tools_reference/settings_reference.rst index ade4ad4a012d2..4483d9a96fb7b 100644 --- a/site/source/docs/tools_reference/settings_reference.rst +++ b/site/source/docs/tools_reference/settings_reference.rst @@ -1185,7 +1185,7 @@ catch and handle ExitStatus exceptions. However, this means all other uncaught exceptions are also caught and re-thrown, which is not always desirable. -Default value: true +Default value: false .. _nodejs_catch_rejection: diff --git a/src/library_browser.js b/src/library_browser.js index 99538d9d5317f..9752d3aaceef8 100644 --- a/src/library_browser.js +++ b/src/library_browser.js @@ -770,13 +770,10 @@ var LibraryBrowser = { emscripten_async_load_script__deps: ['$UTF8ToString'], emscripten_async_load_script: (url, onload, onerror) => { url = UTF8ToString(url); - onload = {{{ makeDynCall('v', 'onload') }}}; - onerror = {{{ makeDynCall('v', 'onerror') }}}; - #if PTHREADS if (ENVIRONMENT_IS_PTHREAD) { err(`emscripten_async_load_script("${url}") failed, emscripten_async_load_script is currently not available in pthreads!`); - return onerror ? onerror() : undefined; + return onerror ? {{{ makeDynCall('v', 'onerror') }}}() : undefined; } #endif #if ASSERTIONS @@ -787,17 +784,20 @@ var LibraryBrowser = { var loadDone = () => { {{{ runtimeKeepalivePop() }}} if (onload) { + var onloadCallback = () => callUserCallback({{{ makeDynCall('v', 'onload') }}}); if (runDependencies > 0) { - dependenciesFulfilled = onload; + dependenciesFulfilled = onloadCallback; } else { - onload(); + onloadCallback(); } } } var loadError = () => { {{{ runtimeKeepalivePop() }}} - onerror?.(); + if (onerror) { + callUserCallback({{{ makeDynCall('v', 'onerror') }}}); + } }; #if ENVIRONMENT_MAY_BE_NODE && DYNAMIC_EXECUTION diff --git a/src/settings.js b/src/settings.js index 4bdc2e8f1a820..b3ead202872f8 100644 --- a/src/settings.js +++ b/src/settings.js @@ -795,7 +795,7 @@ var WASM_EXNREF = false; // desirable. // // [link] -var NODEJS_CATCH_EXIT = true; +var NODEJS_CATCH_EXIT = false; // Catch unhandled rejections in node. This only effect versions of node older // than 15. Without this, old version node will print a warning, but exit diff --git a/test/common.py b/test/common.py index b6a821360304e..36a3ea9198a1e 100644 --- a/test/common.py +++ b/test/common.py @@ -1288,10 +1288,7 @@ def clean_js_output(self, output): long_lines = [] def cleanup(line): - if len(line) > 2048: - # Sanity check that this is really the emscripten program/module on - # a single line. - assert line.startswith('var Module=typeof Module!="undefined"') + if len(line) > 2048 and line.startswith('var Module=typeof Module!="undefined"'): long_lines.append(line) line = '' return line @@ -1366,7 +1363,7 @@ def run_js(self, filename, engine=None, args=None, if assert_returncode == NON_ZERO: self.fail('JS subprocess unexpectedly succeeded (%s): Output:\n%s' % (error.cmd, ret)) else: - self.fail('JS subprocess failed (%s): %s. Output:\n%s' % (error.cmd, error.returncode, ret)) + self.fail('JS subprocess failed (%s): %s (expected=%s). Output:\n%s' % (error.cmd, error.returncode, assert_returncode, ret)) # We should pass all strict mode checks self.assertNotContained('strict warning:', ret) diff --git a/test/test_core.py b/test/test_core.py index 8f9ca5b29b6f2..1075dfbeabe2c 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -5957,7 +5957,7 @@ def test_signals(self): @parameterized({ 'sigint': (EM_SIGINT, 128 + EM_SIGINT, True), - 'sigabrt': (EM_SIGABRT, 7, False) + 'sigabrt': (EM_SIGABRT, 1, False) }) @crossplatform def test_sigaction_default(self, signal, exit_code, assert_identical): diff --git a/test/test_fs_after_main.c b/test/test_fs_after_main.c index f837df085156f..373c919cbc151 100644 --- a/test/test_fs_after_main.c +++ b/test/test_fs_after_main.c @@ -35,6 +35,8 @@ void looper() { fclose(f); } +EM_JS_DEPS(deps, "$safeSetTimeout"); + int main() { EM_ASM({ (function() { @@ -60,10 +62,10 @@ int main() { counter++; if (counter < 5) { out('js queueing'); - setTimeout(looper, 1); + safeSetTimeout(looper, 1); } else { out('js finishing'); - setTimeout(Module['_finish'], 1); + safeSetTimeout(Module['_finish'], 1); } } looper(); diff --git a/test/test_other.py b/test/test_other.py index f4824c6921aa4..ed12a09466eef 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -3836,11 +3836,12 @@ def test_module_exports_with_closure(self): @requires_node def test_node_catch_exit(self): - # Test that in node.js exceptions are not caught if NODEJS_EXIT_CATCH=0 + # Test that in top level JS exceptions are caught and rethrown when NODEJS_EXIT_CATCH=1 + # is set but not by default. create_file('count.c', ''' #include int count(const char *str) { - return (int)strlen(str); + return (int)strlen(str); } ''') @@ -3852,13 +3853,13 @@ def test_node_catch_exit(self): reference_error_text = 'console.log(xxx); //< here is the ReferenceError' - self.run_process([EMCC, 'count.c', '-o', 'count.js']) + self.run_process([EMCC, 'count.c', '-o', 'count.js', '-sNODEJS_CATCH_EXIT=1']) # Check that the ReferenceError is caught and rethrown and thus the original error line is masked self.assertNotContained(reference_error_text, self.run_js('index.js', assert_returncode=NON_ZERO)) - self.run_process([EMCC, 'count.c', '-o', 'count.js', '-sNODEJS_CATCH_EXIT=0']) + self.run_process([EMCC, 'count.c', '-o', 'count.js']) # Check that the ReferenceError is not caught, so we see the error properly self.assertContained(reference_error_text, @@ -6171,16 +6172,17 @@ def test_embed_file_large(self): def test_force_exit(self): create_file('src.c', r''' +#include #include -EMSCRIPTEN_KEEPALIVE void callback() { - EM_ASM({ out('callback pre()') }); +void callback() { + emscripten_out("callback pre()"); emscripten_force_exit(42); - EM_ASM({ out('callback post()') }); + emscripten_out("callback post()"); } int main() { - EM_ASM({ setTimeout(() => { out("calling callback()"); _callback() }, 100) }); + emscripten_async_call(callback, NULL, 100); emscripten_exit_with_live_runtime(); return 123; } @@ -11725,8 +11727,8 @@ def test_assertions_on_reject_promise(self): Promise.reject(); ''') self.run_process([EMCC, test_file('hello_world.c'), '-sASSERTIONS', '--extern-post-js', 'test.js']) - # A return code of 7 is from the unhandled Promise rejection - self.run_js('a.out.js', assert_returncode=7) + # Node exits with 1 on Uncaught Fatal Exception (including unhandled rejections) + self.run_js('a.out.js', assert_returncode=1) def test_on_reject_promise(self): # Check that promise rejections give the correct error code @@ -11734,8 +11736,9 @@ def test_on_reject_promise(self): Promise.reject(); ''') self.run_process([EMCC, test_file('hello_world.c'), '--extern-post-js', 'test.js']) - # A return code of 7 is from the unhandled Promise rejection - self.run_js('a.out.js', assert_returncode=7) + # Node exits with 1 on Uncaught Fatal Exception (including unhandled rejections) + err = self.run_js('a.out.js', assert_returncode=1) + self.assertContained('UnhandledPromiseRejection', err) def test_em_asm_duplicate_strings(self): # We had a regression where tow different EM_ASM strings from two diffferent