GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [v18.x backport] src: add detailed embedder process initialization API by addaleax · Pull Request #44358 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Aug 23, 2022

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 [email protected]
Reviewed-By: Michael Dawson [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot

nodejs-github-bot commented Aug 23, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

nodejs-github-bot commented Aug 23, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Aug 23, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Aug 24, 2022
CI Reliability 2022-08-24 nodejs/reliability#356
Open
32 of 38 tasks
@nodejs-github-bot

nodejs-github-bot commented Aug 24, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Aug 25, 2022
CI Reliability 2022-08-25 nodejs/reliability#357
Open
38 tasks
@nodejs-github-bot

nodejs-github-bot commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung joyeecheung mentioned this pull request Aug 25, 2022
bootstrap: check more metadata when loading the snapshot #44132
Merged
joyeecheung
joyeecheung approved these changes Aug 25, 2022
View reviewed changes
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 25, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 25, 2022
This was referenced Aug 26, 2022
CI Reliability 2022-08-26 nodejs/reliability#358
Open
CI Reliability 2022-08-27 nodejs/reliability#359
Open
CI Reliability 2022-08-28 nodejs/reliability#360
Open
jasnell
jasnell approved these changes Aug 28, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Aug 29, 2022
CI Reliability 2022-08-29 nodejs/reliability#361
Open
30 tasks
@nodejs-github-bot

nodejs-github-bot commented Aug 29, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 30, 2022
CI Reliability 2022-08-30 nodejs/reliability#362
Open
CI Reliability 2022-08-31 nodejs/reliability#363
Open
@nodejs-github-bot

nodejs-github-bot commented Aug 31, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Sep 1, 2022
CI Reliability 2022-09-01 nodejs/reliability#364
Open
28 tasks
@joyeecheung

joyeecheung commented Sep 1, 2022

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Sep 1, 2022

Copy link
Copy Markdown
Member

Opened #44473

addaleax reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Sep 2, 2022
CI Reliability 2022-09-02 nodejs/reliability#365
Open
CI Reliability 2022-09-03 nodejs/reliability#366
Open
CI Reliability 2022-09-04 nodejs/reliability#367
Open
@RafaelGSS
RafaelGSS force-pushed the v18.x-staging branch from d912d2d to 60191c6 Compare September 6, 2022 16:34
@addaleax

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS

RafaelGSS commented Sep 6, 2022

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Sep 7, 2022
CI Reliability 2022-09-07 nodejs/reliability#370
Open
15 tasks
@RafaelGSS
RafaelGSS force-pushed the v18.x-staging branch 2 times, most recently from c0cfb14 to 8ef5c40 Compare September 7, 2022 16:28
@github-actions github-actions Bot mentioned this pull request Sep 8, 2022
CI Reliability 2022-09-08 nodejs/reliability#371
Open
12 tasks
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 8, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 8, 2022
@nodejs-github-bot

nodejs-github-bot commented Sep 8, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Sep 8, 2022

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Sep 9, 2022
CI Reliability 2022-09-09 nodejs/reliability#372
Open
CI Reliability 2022-09-10 nodejs/reliability#373
Open
CI Reliability 2022-09-11 nodejs/reliability#374
Open
CI Reliability 2022-09-12 nodejs/reliability#375
Open
@addaleax

addaleax commented Sep 12, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS

RafaelGSS commented Sep 12, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS
RafaelGSS approved these changes Sep 12, 2022
View reviewed changes

@RafaelGSS RafaelGSS left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mhdawson
mhdawson approved these changes Sep 12, 2022
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@RafaelGSS

RafaelGSS commented Sep 23, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS
RafaelGSS force-pushed the v18.x-staging branch from 99d004e to 6dc0382 Compare September 26, 2022 22:10
@addaleax
src: add detailed embedder process initialization API
8cae5e1
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax
addaleax force-pushed the backport-44121-to-v18.x branch from 5a36fa3 to 8cae5e1 Compare October 1, 2022 13:01
@addaleax

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 1, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 1, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 1, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danielleadams pushed a commit that referenced this pull request Oct 4, 2022
@addaleax @danielleadams
src: add detailed embedder process initialization API
f99f5d3
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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danielleadams

danielleadams commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

Landed in f99f5d3

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@danielleadams danielleadams closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@addaleax @nodejs-github-bot @joyeecheung @RafaelGSS @danielleadams @jasnell @mhdawson
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.