src: add detailed embedder process initialization API #44121

addaleax · 2022-08-03T18:39:39Z

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

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.

nodejs-github-bot · 2022-08-03T18:39:43Z

Review requested:

@nodejs/startup

addaleax

@nodejs/embedders @joyeecheung

src/node.cc

addaleax · 2022-08-03T18:41:29Z

src/node.cc

+      constexpr ULONG first = TRUE;
+      per_process::old_vectored_exception_handler =
+          AddVectoredExceptionHandler(first, TrapWebAssemblyOrContinue);
+    }


Some of this patch may be easier to view with whitespace diff-ing disabled since it adds a few large if blocks.

src/node.cc

addaleax · 2022-08-03T18:46:03Z

src/node_internals.h

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

addaleax · 2022-08-03T18:47:19Z

src/node_main.cc

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


This change does the reverse, move this code to PlatformInit(), but the result is the same.

addaleax · 2022-08-03T18:47:35Z

src/node_main.cc

-  // 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);


Moved to the other stdio initialization code.

src/node.cc

…n API

joyeecheung

Still LGTM

nodejs-github-bot · 2022-08-04T13:24:45Z

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

RaisinTen · 2022-08-04T13:47:47Z

src/node.cc

-InitializationResult InitializeOncePerProcess(int argc, char** argv) {
-  return InitializeOncePerProcess(argc, argv, kDefaultInitialization);
-}
+std::unique_ptr<InitializationResult> InitializeOncePerProcess(


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?

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

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?

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.

Thanks for the additional info.

mhdawson · 2022-08-04T21:47:19Z

@lux01 would be great if you could take a look to see if you have any thoughts/comments from the shared library usage prespective.

mhdawson · 2022-08-04T22:07:42Z

src/node.cc

-      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


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?

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.

@addaleax thanks, if its not documented anywhere I might PR that info our contributing docs somewhere. Does that make sense to you?

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

mhdawson

Overall LGTM with a couple of questions.

…lization API

nodejs-github-bot · 2022-08-05T11:44:21Z

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

mhdawson

LGTM to confirm I'm happy for it to land in current form.

ruyadorno · 2022-08-23T03:21:19Z

I tried again after landing #38905 in the staging branch but it wouldn't land cleanly still. Heads up @addaleax it looks like b697160 will need to be backported in order to land on v18.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: nodejs#44121 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

juanarbol · 2022-10-03T02:03:01Z

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

nodejs/node#44121

* 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>

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. embedding Issues and PRs related to embedding Node.js in another project. labels Aug 3, 2022

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 3, 2022

addaleax commented Aug 3, 2022

View reviewed changes

addaleax added 2 commits August 3, 2022 20:49

fixup! src: add detailed embedder process initialization API

e3cad18

fixup! fixup! src: add detailed embedder process initialization API

c933415

joyeecheung approved these changes Aug 4, 2022

View reviewed changes

src/node.cc Show resolved Hide resolved

src/node.cc Outdated Show resolved Hide resolved

fixup! fixup! fixup! src: add detailed embedder process initializatio…

46057ce

…n API

joyeecheung approved these changes Aug 4, 2022

View reviewed changes

zcbenz approved these changes Aug 4, 2022

View reviewed changes

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2022

RaisinTen reviewed Aug 4, 2022

View reviewed changes

mhdawson reviewed Aug 4, 2022

View reviewed changes

mhdawson approved these changes Aug 4, 2022

View reviewed changes

fixup! fixup! fixup! fixup! src: add detailed embedder process initia…

7973d3d

…lization API

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 5, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 5, 2022

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 5, 2022

mhdawson approved these changes Aug 5, 2022

View reviewed changes

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 5, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 5, 2022

ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Aug 23, 2022

addaleax mentioned this pull request Aug 23, 2022

[v18.x backport] src: add detailed embedder process initialization API #44358

Closed

addaleax added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Aug 23, 2022

RafaelGSS mentioned this pull request Sep 13, 2022

2022-10-18, Version 19.0.0 (Current) #44626

Merged

juanarbol added the dont-land-on-v16.x label Oct 3, 2022

danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Oct 4, 2022

danielleadams mentioned this pull request Oct 11, 2022

v18.11.0 proposal #44968

Merged

jkleinsc added a commit to electron/electron that referenced this pull request Nov 17, 2022

src: add detailed embedder process initialization AP

5f355e0

nodejs/node#44121

codebytere pushed a commit to electron/electron that referenced this pull request Nov 21, 2022

src: add detailed embedder process initialization AP

996d197

nodejs/node#44121

codebytere pushed a commit to electron/electron that referenced this pull request Nov 21, 2022

src: add detailed embedder process initialization AP

60bc147

nodejs/node#44121

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add detailed embedder process initialization API #44121

src: add detailed embedder process initialization API #44121

addaleax commented Aug 3, 2022

nodejs-github-bot commented Aug 3, 2022

addaleax left a comment

addaleax Aug 3, 2022

addaleax Aug 3, 2022

addaleax Aug 3, 2022

addaleax Aug 3, 2022

joyeecheung left a comment

nodejs-github-bot commented Aug 4, 2022

RaisinTen Aug 4, 2022 •

edited

addaleax Aug 4, 2022

mhdawson Aug 4, 2022

addaleax Aug 5, 2022

mhdawson Aug 5, 2022

mhdawson commented Aug 4, 2022

mhdawson Aug 4, 2022

addaleax Aug 5, 2022

mhdawson Aug 5, 2022

addaleax Aug 5, 2022

mhdawson left a comment

nodejs-github-bot commented Aug 5, 2022

mhdawson left a comment

ruyadorno commented Aug 23, 2022 •

edited

juanarbol commented Oct 3, 2022

src: add detailed embedder process initialization API #44121

src: add detailed embedder process initialization API #44121

Conversation

addaleax commented Aug 3, 2022

nodejs-github-bot commented Aug 3, 2022

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 4, 2022

RaisinTen Aug 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 5, 2022

mhdawson left a comment

Choose a reason for hiding this comment

ruyadorno commented Aug 23, 2022 • edited

juanarbol commented Oct 3, 2022

RaisinTen Aug 4, 2022 •

edited

ruyadorno commented Aug 23, 2022 •

edited