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

lib: run prepareMainThreadExecution for third_party_main #26677

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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 addaleax added the embedding Issues and PRs related to embedding Node.js in another project. label Mar 15, 2019
@BridgeAR
Copy link
Member

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

danbev commented Mar 19, 2019

Landed in 81edd0a.

@danbev danbev closed this Mar 19, 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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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.

PR-URL: #26677
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@calebboyd
Copy link
Contributor

calebboyd commented Jun 11, 2019

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' || process.argv[1] === 'debug') {
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

@joyeecheung
Copy link
Member

joyeecheung commented Jun 11, 2019

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.

@calebboyd
Copy link
Contributor

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)

@joyeecheung
Copy link
Member

joyeecheung commented Jun 11, 2019

@calebboyd There are currently two parts of bootstrap

  1. Environment-independent bootstrap that can be snapshotted, currently bootstrap/loaders.js and bootstrap/node.js
  2. 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 addaleax deleted the third_party_main-prepare branch June 11, 2019 21:28
@joyeecheung
Copy link
Member

joyeecheung commented Jun 11, 2019

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

@joyeecheung
Copy link
Member

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

@calebboyd
Copy link
Contributor

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.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 12, 2019

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

@joyeecheung
Copy link
Member

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.

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. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants