Modules bootstrap refactoring - #29937
Conversation
Sorry, something went wrong.
|
@nodejs/modules-active-members This could use some reviews. |
Sorry, something went wrong.
MylesBorins
left a comment
There was a problem hiding this comment.
RSLGTM assuming tests are all passing.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #29937 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #29937 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#29937 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
this lands cleanly on v12.x-staging, but it breaks a test: Command: out/Release/node /home/mzasso/git/nodejs/v12.x/test/parallel/test-repl-tab-complete-nested-repls.js --- TIMEOUT --- [02:19|% 100|+ 2797|- 1]: Done |
Sorry, something went wrong.
|
/cc @bmeck @guybedford could you please take a look... i did some digging and can't figure out what is broken here... seems related to domains 😅 |
Sorry, something went wrong.
|
I had a quick look at this this morning, and it seems like this is some interaction with domain where the error being thrown in run_main_module.js isn't being picked up somehow. If you wrap runMain in a try-catch the error is there fine and synchronously. If you check ExecuteBootstrapper in node.cc the result of the run_main_module call satisfies result.isEmpty() (signalling an error). So somehow the exception handler is just not attaching from there. I'm not sure what code paths this would be corresponding to personally. @joyeecheung @addaleax perhaps you have some ideas on this? |
Sorry, something went wrong.
|
@targos @MylesBorins I finally tracked down where in the PR this code change was coming from. The following patch should fix the issue: diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 7dd0475d48..75300ae942 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -1134,6 +1134,7 @@ Module._extensions['.node'] = function(module, filename) {
Module.runMain = function(main = process.argv[1]) {
const resolvedMain = resolveMainPath(main);
const useESMLoader = shouldUseESMLoader(resolvedMain);
+ module.exports.asyncRunMain = useESMLoader;
if (useESMLoader) {
runMainESM(resolvedMain || main);
} else {
diff --git a/lib/repl.js b/lib/repl.js
index 6e22bd43a9..2871bab27d 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -65,7 +65,8 @@ const path = require('path');
const fs = require('fs');
const { Interface } = require('readline');
const { Console } = require('console');
-const CJSModule = require('internal/modules/cjs/loader').Module;
+const cjsLoader = require('internal/modules/cjs/loader');
+const { Module: CJSModule } = cjsLoader;
const domain = require('domain');
const debug = require('internal/util/debuglog').debuglog('repl');
const {
@@ -1081,6 +1082,8 @@ function complete(line, callback) {
// bufferedCommand.
if (!magic[kBufferedCommandSymbol]) {
magic._domain.on('error', (err) => {
+ if (!cjsLoader.asyncRunMain)
+ throw err;
setImmediate(() => {
throw err;
});
|
Sorry, something went wrong.
PR-URL: #29937 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
@guybedford it looks like the repl test isn't failing on 13.x because #30907 refactored the code that was throwing. Thanks @BridgeAR I don't think we need a backport-pr anymore tbh and have pushed to staging. |
Sorry, something went wrong.
PR-URL: #29937 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This PR includes refactoring parts of the --experimental-modules unflagging in #29866, without actually unflagging --experimental-modules yet, in turn simplifying the unflagging PR.
The main aspect of this is separating out the runMain bootstrap from CJS, and very carefully ensuring that the async bootstrap and modules promise creation only applies when absolutely necessary, to avoid any async hooks noise.
The various other changes are there to fix test cases that fail under unflagging, I will provide context with code comments below.
Checklist