src: don't run bootstrapper in CreateEnvironment by codebytere · Pull Request #31910 · nodejs/node

codebytere · 2020-02-22T07:17:27Z

#26788 added a call to prepareMainThreadExecution to CreateEnvironment, which would prepare the main thread for execution any time an embedder created a new environment. However, this caused an unfortunate doubling-up effect, and meant that Electron would see stuff like:

> process.emit('warning', new Error('warn'))
(node:55797) Error: warn
(node:55797) Error: warn

Since both execution pathways (env and whatever the actual execution was) were being exercised. To fix this, I believe we should remove bootstrapping code from CreateEnvironment.

cc @joyeecheung @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

gireeshpunathil · 2020-02-22T07:22:30Z

where is the other bootstrapping that is happening?

codebytere · 2020-02-22T07:24:51Z

@gireeshpunathil:

node/src/node.cc

Lines 404 to 448 in 84b8857

    
           MaybeLocal<Value> StartMainThreadExecution(Environment* env) { 
        
             // To allow people to extend Node in different ways, this hook allows 
        
             // one to drop a file lib/_third_party_main.js into the build 
        
             // directory which will be executed instead of Node's normal loading. 
        
             if (NativeModuleEnv::Exists("_third_party_main")) { 
        
               return StartExecution(env, "internal/main/run_third_party_main"); 
        
             } 
        
             std::string first_argv; 
        
             if (env->argv().size() > 1) { 
        
               first_argv = env->argv()[1]; 
        
             } 
        
             if (first_argv == "inspect" || first_argv == "debug") { 
        
               return StartExecution(env, "internal/main/inspect"); 
        
             } 
        
             if (per_process::cli_options->print_help) { 
        
               return StartExecution(env, "internal/main/print_help"); 
        
             } 
        
             if (env->options()->prof_process) { 
        
               return StartExecution(env, "internal/main/prof_process"); 
        
             } 
        
             // -e/--eval without -i/--interactive 
        
             if (env->options()->has_eval_string && !env->options()->force_repl) { 
        
               return StartExecution(env, "internal/main/eval_string"); 
        
             } 
        
             if (env->options()->syntax_check_only) { 
        
               return StartExecution(env, "internal/main/check_syntax"); 
        
             } 
        
             if (!first_argv.empty() && first_argv != "-") { 
        
               return StartExecution(env, "internal/main/run_main_module"); 
        
             } 
        
             if (env->options()->force_repl || uv_guess_handle(STDIN_FILENO) == UV_TTY) { 
        
               return StartExecution(env, "internal/main/repl"); 
        
             } 
        
             return StartExecution(env, "internal/main/eval_stdin"); 
        
           }

See that all those js files call prepareMainThreadExecution(true).

himself65 · 2020-02-22T10:21:47Z

related: #31909

test/cctest/test_environment.cc

joyeecheung · 2020-02-22T14:09:46Z

By the way, I think a quick band-aid fix to this issue without introducing breaking changes would be to maintain something like isInitialized in pre_execution.js for prepareMainThreadExecution() and return early there if it's set to true. It's not ideal but would lead to less impact.

addaleax

LGTM

#30467 contains a non-breaking version of this along the lines of what Joyee suggested, but given that I don’t see how the current state is usable, I’m good with landing this as a semver-patch change.

nodejs-github-bot · 2020-02-22T23:18:36Z

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

nodejs-github-bot · 2020-02-24T23:23:05Z

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

nodejs-github-bot · 2020-02-26T16:45:21Z

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

src/node_internals.h

nodejs-github-bot · 2020-02-27T05:59:41Z

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

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere · 2020-02-27T21:50:03Z

Landed in 65e18a8

addaleax · 2020-02-27T22:05:00Z

🎉 Thanks for landing this!

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Refs: nodejs#31910

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Refs: #31910 PR-URL: #30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs/node#31910

Refs: nodejs#31910 PR-URL: nodejs#30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Refs: #31910 Backport-PR-URL: #35241 PR-URL: #30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

codebytere requested review from addaleax and joyeecheung February 22, 2020 07:17

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 22, 2020

joyeecheung reviewed Feb 22, 2020

View reviewed changes

test/cctest/test_environment.cc Outdated Show resolved Hide resolved

addaleax approved these changes Feb 22, 2020

View reviewed changes

codebytere changed the title ~~src,lib: don't run bootstrapper in CreateEnvironment~~ src: don't run bootstrapper in CreateEnvironment Feb 26, 2020

codebytere requested a review from joyeecheung February 26, 2020 16:44

codebytere force-pushed the createenv-remove-some=bootstrap branch from 20a67b2 to 26a8571 Compare February 26, 2020 16:44

joyeecheung approved these changes Feb 26, 2020

View reviewed changes

src/node_internals.h Outdated Show resolved Hide resolved

src: don't run bootstrapper in CreateEnvironment

57fe1c6

codebytere force-pushed the createenv-remove-some=bootstrap branch from 26a8571 to 57fe1c6 Compare February 27, 2020 05:58

codebytere added a commit that referenced this pull request Feb 27, 2020

src: don't run bootstrapper in CreateEnvironment

65e18a8

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere closed this Feb 27, 2020

codebytere deleted the createenv-remove-some=bootstrap branch February 27, 2020 21:50

addaleax mentioned this pull request Feb 28, 2020

src: improve embedder API #30467

Closed

4 tasks

codebytere added a commit that referenced this pull request Feb 29, 2020

src: don't run bootstrapper in CreateEnvironment

6a35b0d

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere mentioned this pull request Feb 29, 2020

v13.10.0 proposal #32027

Merged

addaleax added a commit to addaleax/node that referenced this pull request Mar 12, 2020

test: re-enable cctest that was commented out

c0b957c

Refs: nodejs#31910

codebytere added a commit that referenced this pull request Mar 15, 2020

src: don't run bootstrapper in CreateEnvironment

b7f8042

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere added a commit that referenced this pull request Mar 17, 2020

src: don't run bootstrapper in CreateEnvironment

9ca5787

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere mentioned this pull request Mar 17, 2020

v12.16.2 proposal #32313

Merged

addaleax added a commit that referenced this pull request Mar 21, 2020

test: re-enable cctest that was commented out

2561484

Refs: #31910 PR-URL: #30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

codebytere added a commit that referenced this pull request Mar 30, 2020

src: don't run bootstrapper in CreateEnvironment

041408d

PR-URL: #31910 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere added a commit to electron/electron that referenced this pull request Apr 13, 2020

Remove prepareMainThreadExecution patch

dec0771

nodejs/node#31910

joyeecheung mentioned this pull request Apr 20, 2020

Something wrong with coverage stats recently? #32912

Closed

addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020

test: re-enable cctest that was commented out

bb689cf

Refs: nodejs#31910 PR-URL: nodejs#30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

addaleax added a commit that referenced this pull request Sep 23, 2020

test: re-enable cctest that was commented out

f325c95

Refs: #31910 Backport-PR-URL: #35241 PR-URL: #30467 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: don't run bootstrapper in CreateEnvironment#31910

src: don't run bootstrapper in CreateEnvironment#31910
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:createenv-remove-some=bootstrap

codebytere commented Feb 22, 2020 •

edited

Loading

Uh oh!

gireeshpunathil commented Feb 22, 2020

Uh oh!

codebytere commented Feb 22, 2020 •

edited

Loading

Uh oh!

himself65 commented Feb 22, 2020

Uh oh!

Uh oh!

joyeecheung commented Feb 22, 2020

Uh oh!

addaleax left a comment

Uh oh!

nodejs-github-bot commented Feb 22, 2020

Uh oh!

nodejs-github-bot commented Feb 24, 2020

Uh oh!

nodejs-github-bot commented Feb 26, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 27, 2020

Uh oh!

codebytere commented Feb 27, 2020

Uh oh!

addaleax commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

codebytere commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

gireeshpunathil commented Feb 22, 2020

Uh oh!

codebytere commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himself65 commented Feb 22, 2020

Uh oh!

Uh oh!

joyeecheung commented Feb 22, 2020

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 22, 2020

Uh oh!

nodejs-github-bot commented Feb 24, 2020

Uh oh!

nodejs-github-bot commented Feb 26, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 27, 2020

Uh oh!

codebytere commented Feb 27, 2020

Uh oh!

addaleax commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codebytere commented Feb 22, 2020 •

edited

Loading

codebytere commented Feb 22, 2020 •

edited

Loading