[v18.x backport] src: add detailed embedder process initialization API #44358
Conversation
This file contains 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
|
Review requested:
|
nodejs-github-bot
added
c++
38 tasks
38 tasks
joyeecheung
approved these changes
Aug 25, 2022
github-actions
bot
removed
the
request-ci
This was referenced Aug 26, 2022
jasnell
approved these changes
Aug 28, 2022
30 tasks
This was referenced Aug 30, 2022
28 tasks
|
Looks like the recent flakes are also present in v18.x staging now, we should probably backport those test fixes there too.. |
|
Opened #44473 |
This was referenced Sep 2, 2022
RafaelGSS
force-pushed
the
v18.x-staging
branch
from
d912d2d
to
60191c6
Compare
|
@RafaelGSS Are the failures here expected? #44473 was closed but this still doesn't seem like it's related to the backport in any way. |
I'm not sure. The #44473 didn't land. @joyeecheung are you sure all the patches are in v18.x-staging? Basically, the proposal contains everything from this list: |
15 tasks
RafaelGSS
force-pushed
the
v18.x-staging
branch
2 times, most recently
from
c0cfb14
to
8ef5c40
Compare
12 tasks
github-actions
bot
removed
the
request-ci
|
@RafaelGSS Yes, I think the tests fixes have been landed. The new test failures were caused by #44402 and #44366, those probably need some proper fix (maybe #44571 does it for the watch mode failure) |
This was referenced Sep 9, 2022
|
@nodejs/backporters Can this be merged? As in #44571 this is something I’d leave to y’all unless you’d prefer for me to land this. |
|
I think so. I'll do the next regular release, in case it conflicts somehow I'll let you know. |
|
@addaleax I'm preparing the next v18 release. Could you please rebase to solve the conflicts? |
RafaelGSS
force-pushed
the
v18.x-staging
branch
from
99d004e
to
6dc0382
Compare
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>
addaleax
force-pushed
the
backport-44121-to-v18.x
branch
from
5a36fa3
to
8cae5e1
Compare
|
@RafaelGSS Yes, done. It’s highly frustrating that backport PRs just lay around as open PRs with no conflicts and green CI for weeks, and then miss a release because they’ve started having a conflict with a later backport and I happened to have been unavailable for a work week. This sucks. |
github-actions
bot
removed
the
request-ci
danielleadams
pushed a commit
that referenced
this pull request
Oct 4, 2022
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>
|
Landed in f99f5d3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #44121, only conflict was a minor conflict in node.h with #43629
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
TODOto add it to the public API in Node.js.This commit addresses that
TODO, andTODOs around theNODE_SHARED_MODEusage. Specifically:InitializeOncePerProcess()andTearDownOncePerProcess()are added to the public API.
flagsoption of these functions are merged with theflagsoption 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).
TODOfor turningInitializeNodeWithArgs()intoa small wrapper around
InitializeOncePerProcess()andeventually removing it (at least one major release cycle
each, presumably).
NODE_SHARED_MODEguards and replace them withruntime options.
PR-URL: #44121
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com