-
Notifications
You must be signed in to change notification settings - Fork 30k
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
src: add detailed embedder process initialization API #44121
src: add detailed embedder process initialization API #44121
Conversation
So far, process initialization has been a bit all over the place in Node.js. `InitializeNodeWithArgs()` is our main public API for this, but inclusion of items in it vs. `InitializeOncePerProcess()` and `PlatformInit()` has been random at best. Likewise, some pieces of initialization have been guarded by `NODE_SHARED_MODE`, but also fairly randomly and without any meaningful connection to shared library usage. This leaves embedders in a position to cherry-pick some of the initialization code into their own code to make their application behave like typical Node.js applications to the degree to which they desire it. Electron takes an alternative route and makes direct use of `InitializeOncePerProcess()` already while it is a private API, with a `TODO` to add it to the public API in Node.js. This commit addresses that `TODO`, and `TODO`s around the `NODE_SHARED_MODE` usage. Specifically: - `InitializeOncePerProcess()` and `TearDownOncePerProcess()` are added to the public API. - The `flags` option of these functions are merged with the `flags` option for `InitializeNodeWithArgs()`, since they essentially share the same semantics. - The return value of the function is made an abstract class, rather than a struct, for easier API/ABI stability. - Initialization code from `main()` is brought into these functions (since that makes sense in general). - Add a `TODO` for turning `InitializeNodeWithArgs()` into a small wrapper around `InitializeOncePerProcess()` and eventually removing it (at least one major release cycle each, presumably). - Remove `NODE_SHARED_MODE` guards and replace them with runtime options.
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nodejs/embedders @joyeecheung
constexpr ULONG first = TRUE; | ||
per_process::old_vectored_exception_handler = | ||
AddVectoredExceptionHandler(first, TrapWebAssemblyOrContinue); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of this patch may be easier to view with whitespace diff-ing disabled since it adds a few large if
blocks.
int exit_code_ = 0; | ||
std::vector<std::string> args_; | ||
std::vector<std::string> exec_args_; | ||
std::vector<std::string> errors_; | ||
bool early_return_ = false; | ||
MultiIsolatePlatform* platform_ = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be writable accessors as well for these but I think as long as these values are only ever set from internal usage, this should be fine.
// build. In order to run test cases against shared lib build, we also need | ||
// to do the same thing for shared lib build here, but only for SIGPIPE for | ||
// now. If node::PlatformInit() is moved to here, then this section could be | ||
// removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does the reverse, move this code to PlatformInit()
, but the result is the same.
// Disable stdio buffering, it interacts poorly with printf() | ||
// calls elsewhere in the program (e.g., any logging from V8.) | ||
setvbuf(stdout, nullptr, _IONBF, 0); | ||
setvbuf(stderr, nullptr, _IONBF, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the other stdio initialization code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
InitializationResult InitializeOncePerProcess(int argc, char** argv) { | ||
return InitializeOncePerProcess(argc, argv, kDefaultInitialization); | ||
} | ||
std::unique_ptr<InitializationResult> InitializeOncePerProcess( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand why this is wrapping the InitializationResult in a unique_ptr now. Is it because of:
The return value of the function is made an abstract class,
rather than a struct, for easier API/ABI stability.
? Does it mean that we will be able to change the InitializationResult
part into something else in the future if the need arises, without breaking ABI compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because of:
The return value of the function is made an abstract class,
rather than a struct, for easier API/ABI stability.?
Exactly 👍
Does it mean that we will be able to change the
InitializationResult
part into something else in the future if the need arises, without breaking ABI compatibility?
Well, there are always limitations, but we could add new fields or change the internal representation of existing fields without breaking ABI compatibility. (There’s also more extensive solutions to this, of course, e.g. how the CommonEnvironmentSetup
class avoids virtual functions and instead uses an internal struct to define its properties. I think that would be a bit much here but I’m happy to move to that model if that’s preferred.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other places in the embedder API where we don't go as far as CommonEnvironmentSetup? If we want to get to ABI stabilty in the embedder API I guess the question would be why not do that when InitializationResult is being added to the externa API if its possible/not an unreasonable amount of work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other places in the embedder API where we don't go as far as CommonEnvironmentSetup?
We have numerous virtual classes, yes.
why not do that when InitializationResult is being added to the externa API
It’s a bit easier this way, because the instance is accessed from a number of different places in our code. It doesn’t matter much, really – we can make changes while keeping ABI stability fairly easily either way.
I’ve added a small change in 7973d3d to make sure that there is no way to create instances of this class externally. That should rule out any theoretical concerns about userland subclasses (where virtual classes do indeed become a bit more difficult ABI-wise).
@mhdawson Please take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional info.
@lux01 would be great if you could take a look to see if you have any thoughts/comments from the shared library usage prespective. |
result.early_return = true; | ||
fprintf(stderr, "OpenSSL configuration error:\n"); | ||
ERR_print_errors_fp(stderr); | ||
// XXX: ERR_GET_REASON does not return something that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grepped under doc/contributing for XXX and TODO to see if we've documented when to use one versus the other but could not find any guidance. Is it documented somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, XXX
is “this is odd (and there might be a better alternative)” and TODO
is “there is definitely a better alternative”. Ultimately, not a big difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax thanks, if its not documented anywhere I might PR that info our contributing docs somewhere. Does that make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhdawson Sure, go for it! I think it's also fine to adjust the definitions if other collaborators feel that these (should) have different meanings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM with a couple of questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to confirm I'm happy for it to land in current form.
So far, process initialization has been a bit all over the place in Node.js. `InitializeNodeWithArgs()` is our main public API for this, but inclusion of items in it vs. `InitializeOncePerProcess()` and `PlatformInit()` has been random at best. Likewise, some pieces of initialization have been guarded by `NODE_SHARED_MODE`, but also fairly randomly and without any meaningful connection to shared library usage. This leaves embedders in a position to cherry-pick some of the initialization code into their own code to make their application behave like typical Node.js applications to the degree to which they desire it. Electron takes an alternative route and makes direct use of `InitializeOncePerProcess()` already while it is a private API, with a `TODO` to add it to the public API in Node.js. This commit addresses that `TODO`, and `TODO`s around the `NODE_SHARED_MODE` usage. Specifically: - `InitializeOncePerProcess()` and `TearDownOncePerProcess()` are added to the public API. - The `flags` option of these functions are merged with the `flags` option for `InitializeNodeWithArgs()`, since they essentially share the same semantics. - The return value of the function is made an abstract class, rather than a struct, for easier API/ABI stability. - Initialization code from `main()` is brought into these functions (since that makes sense in general). - Add a `TODO` for turning `InitializeNodeWithArgs()` into a small wrapper around `InitializeOncePerProcess()` and eventually removing it (at least one major release cycle each, presumably). - Remove `NODE_SHARED_MODE` guards and replace them with runtime options. PR-URL: nodejs#44121 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
So far, process initialization has been a bit all over the place in Node.js. `InitializeNodeWithArgs()` is our main public API for this, but inclusion of items in it vs. `InitializeOncePerProcess()` and `PlatformInit()` has been random at best. Likewise, some pieces of initialization have been guarded by `NODE_SHARED_MODE`, but also fairly randomly and without any meaningful connection to shared library usage. This leaves embedders in a position to cherry-pick some of the initialization code into their own code to make their application behave like typical Node.js applications to the degree to which they desire it. Electron takes an alternative route and makes direct use of `InitializeOncePerProcess()` already while it is a private API, with a `TODO` to add it to the public API in Node.js. This commit addresses that `TODO`, and `TODO`s around the `NODE_SHARED_MODE` usage. Specifically: - `InitializeOncePerProcess()` and `TearDownOncePerProcess()` are added to the public API. - The `flags` option of these functions are merged with the `flags` option for `InitializeNodeWithArgs()`, since they essentially share the same semantics. - The return value of the function is made an abstract class, rather than a struct, for easier API/ABI stability. - Initialization code from `main()` is brought into these functions (since that makes sense in general). - Add a `TODO` for turning `InitializeNodeWithArgs()` into a small wrapper around `InitializeOncePerProcess()` and eventually removing it (at least one major release cycle each, presumably). - Remove `NODE_SHARED_MODE` guards and replace them with runtime options. PR-URL: nodejs#44121 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
So far, process initialization has been a bit all over the place in Node.js. `InitializeNodeWithArgs()` is our main public API for this, but inclusion of items in it vs. `InitializeOncePerProcess()` and `PlatformInit()` has been random at best. Likewise, some pieces of initialization have been guarded by `NODE_SHARED_MODE`, but also fairly randomly and without any meaningful connection to shared library usage. This leaves embedders in a position to cherry-pick some of the initialization code into their own code to make their application behave like typical Node.js applications to the degree to which they desire it. Electron takes an alternative route and makes direct use of `InitializeOncePerProcess()` already while it is a private API, with a `TODO` to add it to the public API in Node.js. This commit addresses that `TODO`, and `TODO`s around the `NODE_SHARED_MODE` usage. Specifically: - `InitializeOncePerProcess()` and `TearDownOncePerProcess()` are added to the public API. - The `flags` option of these functions are merged with the `flags` option for `InitializeNodeWithArgs()`, since they essentially share the same semantics. - The return value of the function is made an abstract class, rather than a struct, for easier API/ABI stability. - Initialization code from `main()` is brought into these functions (since that makes sense in general). - Add a `TODO` for turning `InitializeNodeWithArgs()` into a small wrapper around `InitializeOncePerProcess()` and eventually removing it (at least one major release cycle each, presumably). - Remove `NODE_SHARED_MODE` guards and replace them with runtime options. PR-URL: nodejs#44121 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
As described in the comments, this depends on #38905; which can not be landed in v16.x |
So far, process initialization has been a bit all over the place in Node.js. `InitializeNodeWithArgs()` is our main public API for this, but inclusion of items in it vs. `InitializeOncePerProcess()` and `PlatformInit()` has been random at best. Likewise, some pieces of initialization have been guarded by `NODE_SHARED_MODE`, but also fairly randomly and without any meaningful connection to shared library usage. This leaves embedders in a position to cherry-pick some of the initialization code into their own code to make their application behave like typical Node.js applications to the degree to which they desire it. Electron takes an alternative route and makes direct use of `InitializeOncePerProcess()` already while it is a private API, with a `TODO` to add it to the public API in Node.js. This commit addresses that `TODO`, and `TODO`s around the `NODE_SHARED_MODE` usage. Specifically: - `InitializeOncePerProcess()` and `TearDownOncePerProcess()` are added to the public API. - The `flags` option of these functions are merged with the `flags` option for `InitializeNodeWithArgs()`, since they essentially share the same semantics. - The return value of the function is made an abstract class, rather than a struct, for easier API/ABI stability. - Initialization code from `main()` is brought into these functions (since that makes sense in general). - Add a `TODO` for turning `InitializeNodeWithArgs()` into a small wrapper around `InitializeOncePerProcess()` and eventually removing it (at least one major release cycle each, presumably). - Remove `NODE_SHARED_MODE` guards and replace them with runtime options. PR-URL: #44121 Backport-PR-URL: #44358 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Notable changes: * cli: * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366 * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631
Notable changes: * cli: * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366 * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
Notable changes: * cli: * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366 * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in [#44366](#44366) Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in [#44366](#44366) Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in [#44366](#44366) Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in [#44366](#44366) Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in #44366 Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in #44366 Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
watch mode (experimental): Running in 'watch' mode using `node --watch` restarts the process when an imported file is changed. Contributed by Moshe Atlow in #44366 Other notable changes: * fs: * (SEMVER-MINOR) add `FileHandle.prototype.readLines` (Antoine du Hamel) #42590 * http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * http2: * (SEMVER-MINOR) make early hints generic (Yagiz Nizipli) #44820 * lib: * (SEMVER-MINOR) refactor transferable AbortSignal (flakey5) #44048 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * util: * (SEMVER-MINOR) add default value option to parsearg (Manuel Spigolon) #44631 PR-URL: #44968
* chore: bump node in DEPS to v18.12.1 * chore: update patches * chore: add missing <algorithm> include * src: add detailed embedder process initialization AP nodejs/node#44121 * chore: update gn build files * dns: support dns module in the snapshot nodejs/node#44633 #36118 * src: fix OOB reads in process.title getter nodejs/node#31633 * chore: fix incorrectly removed patch bit Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v18.12.1 * chore: update patches * chore: add missing <algorithm> include * src: add detailed embedder process initialization AP nodejs/node#44121 * chore: update gn build files * dns: support dns module in the snapshot nodejs/node#44633 electron#36118 * src: fix OOB reads in process.title getter nodejs/node#31633 * chore: fix incorrectly removed patch bit Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
So far, process initialization has been a bit all over the place
in Node.js.
InitializeNodeWithArgs()
is our main public APIfor this, but inclusion of items in it vs.
InitializeOncePerProcess()
and
PlatformInit()
has been random at best. Likewise,some pieces of initialization have been guarded by
NODE_SHARED_MODE
, but also fairly randomly and withoutany meaningful connection to shared library usage.
This leaves embedders in a position to cherry-pick some of
the initialization code into their own code to make their
application behave like typical Node.js applications to the
degree to which they desire it.
Electron takes an alternative route and makes direct use of
InitializeOncePerProcess()
already while it is a privateAPI, with a
TODO
to add it to the public API in Node.js.This commit addresses that
TODO
, andTODO
s around theNODE_SHARED_MODE
usage. Specifically:InitializeOncePerProcess()
andTearDownOncePerProcess()
are added to the public API.
flags
option of these functions are merged with theflags
option forInitializeNodeWithArgs()
, since theyessentially share the same semantics.
rather than a struct, for easier API/ABI stability.
main()
is brought into thesefunctions (since that makes sense in general).
TODO
for turningInitializeNodeWithArgs()
intoa small wrapper around
InitializeOncePerProcess()
andeventually removing it (at least one major release cycle
each, presumably).
NODE_SHARED_MODE
guards and replace them withruntime options.