Modules bootstrap refactoring

[guybedford]

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

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[Trott]

@nodejs/modules-active-members This could use some reviews.

[MylesBorins]

RSLGTM assuming tests are all passing.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26018/

test was fixed hooray!

[guybedford]

Landed in a6b030d.

[targos]

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 

[MylesBorins]

/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 😅

[guybedford]

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?

[guybedford]

@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}); 

[MylesBorins]

@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.