FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v18.x backport] src: add detailed embedder process initialization API by addaleax · Pull Request #44358 · nodejs/node · GitHub

/ node Public

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

Closed
addaleax wants to merge 1 commit into
nodejs:v18.x-stagingfrom
addaleax:backport-44121-to-v18.x
Closed

[v18.x backport] src: add detailed embedder process initialization API#44358
addaleax wants to merge 1 commit into
nodejs:v18.x-stagingfrom
addaleax:backport-44121-to-v18.x

Conversation

Copy link
Copy Markdown
Member

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

PR-URL: #44121
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

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. v18.x labels Aug 23, 2022

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2022

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member

Looks like the recent flakes are also present in v18.x staging now, we should probably backport those test fixes there too..

Copy link
Copy Markdown
Member

Opened #44473

addaleax commented Sep 6, 2022

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

branch-diff upstream/v18.x-staging upstream/main --exclude-label=semver-major,dont-land-on-v18.x,backport-requested-v18.x,backport-blocked-v18.x,backport-open-v18.x,backported-to-v18.x --filter-release --format=simple --reverse

RafaelGSS force-pushed the v18.x-staging branch 2 times, most recently from c0cfb14 to 8ef5c40 Compare September 7, 2022 16:28
addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

I think so. I'll do the next regular release, in case it conflicts somehow I'll let you know.

RafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

RafaelGSS commented Sep 23, 2022
edited
Loading

Copy link
Copy Markdown
Member

@addaleax I'm preparing the next v18 release. Could you please rebase to solve the conflicts?

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 October 1, 2022 13:01

addaleax commented Oct 1, 2022

Copy link
Copy Markdown
Member Author

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

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2022

Copy link
Copy Markdown
Collaborator

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>

Copy link
Copy Markdown
Contributor

Landed in f99f5d3

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL