lib: run prepareMainThreadExecution for third_party_main by addaleax · Pull Request #26677 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Treat _third_party_main like any other CJS entry point, as it
was done before 6967f91.

Checklist

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

Treat _third_party_main like any other CJS entry point, as it was done before 6967f91.

addaleax added the embedding

Issues and PRs related to embedding Node.js in another project.

label

Mar 15, 2019

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 15, 2019

danbev pushed a commit that referenced this pull request

Mar 19, 2019

Treat _third_party_main like any other CJS entry point, as it was done before 6967f91.

PR-URL: #26677 Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Gus Caplan me@gus.host Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Richard Lau riclau@uk.ibm.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

targos pushed a commit to targos/node that referenced this pull request

Mar 27, 2019

Treat _third_party_main like any other CJS entry point, as it was done before 6967f91.

PR-URL: nodejs#26677 Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Gus Caplan me@gus.host Reviewed-By: Joyee Cheung joyeec9h3@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Richard Lau riclau@uk.ibm.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

targos pushed a commit that referenced this pull request

Mar 27, 2019

Treat _third_party_main like any other CJS entry point, as it was done before 6967f91.

A ~~lot~~ little of the setup in prepareMainThreadExecution actually never ran with the previous implementation. (_t_p_m wasn't the same as other modules). Specifically, cluster setup.

see:

if (NativeModule.exists('_third_party_main')) {
process.nextTick(() => {
NativeModule.require('_third_party_main');
});
return;
}

// `node inspect ...` or `node debug ...`
if (process.argv[1] === 'inspect' \|
if (process.argv[1] === 'debug') {
process.emitWarning(
'`node debug` is deprecated. Please use `node inspect` instead.',
'DeprecationWarning', 'DEP0068');
}

// Start the debugger agent.
process.nextTick(() => {
NativeModule.require('internal/deps/node-inspect/lib/_inspect').start();
});
return;
}

// `node --prof-process`
// TODO(joyeecheung): use internal/options instead of process.profProcess
if (process.profProcess) {
NativeModule.require('internal/v8_prof_processor');
return;
}

// There is user code to be run.
prepareUserCodeExecution();
executeUserCode();
}

function prepareUserCodeExecution() {
// If this is a worker in cluster mode, start up the communication
// channel. This needs to be done before any user code gets executed
// (including preload modules).
if (process.argv[1] && process.env.NODE_UNIQUE_ID) {
const cluster = NativeModule.require('cluster');
cluster._setupWorker();
// Make sure it's not accidentally inherited by child processes.
delete process.env.NODE_UNIQUE_ID;
}

I'm not sure if this is a bug or a breaking change, because it is now commented as legacy..

ref: nexe/nexe#638

What are the preparations that do make sense for _third_party_main.js? I would think the cluster setup code is necessary, but the correct timing for it to be run may be sensitive to how embedders manage process.argv[1] and process.env.

I think anything that doesn't stem from user code would qualify as a preparation. Cluster setup is dependent on user-code usage of cluster. So the effects are occurring before the embedded _third_party_main.js is run. I can't think of any other scenario where this might be an issue, just the cluster setup. (_setupWorker)

@calebboyd There are currently two parts of bootstrap

Environment-independent bootstrap that can be snapshotted, currently bootstrap/loaders.js and bootstrap/node.js
Environment-dependent bootstrap (i.e. depending on CLI flags, environment variables), currently in bootstrap/pre_execution.js which includes prepareMainThreadExecution.

IIUC you are talking about doing 2 after running _third_party_main.js? Off the top of my head, that seems reasonable, since that's what we have been doing before this PR. However this does break certain usage of _third_party_main.js - for instance, process is currently incomplete before prepareMainThreadExecution (because things like process.argv depends on how user launch the process). There should probably be more phases in terms of what gets to be done when, but the issue is we don't really know how _third_party_main.js is actually used in the wild and the contract is pretty vague at the moment, as people who use this trick usually also have the ability to patch the source.

So I guess my question is, what is nexe doing with _third_party_main.js? (Also what else does it do around that point?)

addaleax deleted the third_party_main-prepare branch

June 11, 2019 21:28

@calebboyd BTW, the intent of this PR is to make it so that _third_party_main.js is run as if the program is started like node <embedder-cli-flags> _third_party_main.js - from what I can tell, the use case of nexe is more like node -r _third_party_main.js <embedder-cli-flags> <user-app-main>?

Also, we previously thought about deprecating _third_party_main.js #24017 - would be very helpful if you can talk about your use cases there.

I don't think there is anything wrong with node <embedder-cli-flags> _third_party_main.js

This is actually fine for the nexe use case. Historically though the bootstrap procedure would do all the setup except for cluster -- which would run immediately before user code execution

prepareUserCodeExecution();
executeUserCode();

I would propose that for the same reasons cluster setup is run before preload modules, _third_party_main.js should be run before cluster setup.

@calebboyd Do you know why running _third_party_main.js after the cluster set up code breaks nexe? We could move that but then it's better to actually find out why it has to be done in that order and add some comments there. Or otherwise it may break again once someone touch the code transitively executed by the first require('cluster'), etc - there are actually several side-effects.

The cluster set up code mostly just instantiates certain variables exported by cluster depending on the value of process.env.NODE_UNIQUE_ID, and then initializes the IPC channel, then deletes process.env.NODE_UNIQUE_ID. Does nexe's _third_party_main.js run any user code? Theoretically, no user code should be run before this setup (see the comments there), or they could e.g. spawn a child process which inherits process.env.NODE_UNIQUE_ID then that's not going to be unique.

Also, do you modify process.argv[1], by any chance? If that is falsy until _third_party_main.js is run, then with the current order _setupWorker() is not going to be invoked for the worker script. However, the embedder should be able to control process.argv[1] from the C++ layer before running the bootstrap scripts.

This was referenced

Mar 11, 2021

This was referenced

May 25, 2021

This was referenced

Apr 7, 2022

This was referenced

Jan 31, 2023

Reviewers

jasnell jasnell approved these changes