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

Modules bootstrap refactoring #29937

Closed
wants to merge 14 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 11, 2019

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 11, 2019
lib/repl.js Show resolved Hide resolved
lib/internal/main/main.js Outdated Show resolved Hide resolved
lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved
lib/internal/main/main.js Outdated Show resolved Hide resolved
@Fishrock123 Fishrock123 added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. labels Oct 12, 2019
@Trott
Copy link
Member

Trott commented Oct 13, 2019

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

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM assuming tests are all passing.

@nodejs-github-bot
Copy link
Collaborator

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/process/esm_loader.js Show resolved Hide resolved
lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved
lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved
test/addons/async-hooks-promise/test.js Outdated Show resolved Hide resolved
test/addons/async-hooks-promise/test.js Outdated Show resolved Hide resolved
@guybedford guybedford requested a review from Trott October 16, 2019 18:15
@Trott Trott dismissed their stale review October 16, 2019 18:28

test was fixed hooray!

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2019
guybedford pushed a commit that referenced this pull request Oct 17, 2019
PR-URL: #29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in a6b030d.

@guybedford guybedford closed this Oct 17, 2019
@guybedford guybedford deleted the async-bootstrap-refactor branch October 17, 2019 01:51
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
PR-URL: #29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
PR-URL: #29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 7, 2020
PR-URL: nodejs#29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos
Copy link
Member

targos commented Jan 8, 2020

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
Copy link
Contributor

/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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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 pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins
Copy link
Contributor

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

BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29937
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants