Skip to content
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: define per-isolate internal bindings registration callback #45547

Closed

Conversation

legendecas
Copy link
Member

Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

Refs: #42528

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 20, 2022
@legendecas legendecas changed the title src: define realm-aware internal bindings src: define per-isolate internal bindings registration callback Nov 22, 2022
@legendecas legendecas force-pushed the shadow-realm/per-realm-binding branch 2 times, most recently from 06e2c24 to bbecbcd Compare November 22, 2022 07:52
@legendecas legendecas marked this pull request as ready for review November 22, 2022 17:26
src/env-inl.h Outdated Show resolved Hide resolved
src/node_binding.cc Outdated Show resolved Hide resolved
src/node_builtins.cc Outdated Show resolved Hide resolved
@legendecas legendecas force-pushed the shadow-realm/per-realm-binding branch 2 times, most recently from f762397 to e07122b Compare November 29, 2022 18:01
src/node_binding.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_builtins.cc Outdated Show resolved Hide resolved
@legendecas legendecas force-pushed the shadow-realm/per-realm-binding branch from e07122b to 0dc40ad Compare December 6, 2022 14:52
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

src/util.cc Show resolved Hide resolved
@legendecas
Copy link
Member Author

@nodejs/realm PTAL, thank you!

src/node_binding.cc Show resolved Hide resolved
src/node_binding.h Outdated Show resolved Hide resolved
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.
Create worker binding templates in the per-isolate initialization.
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the realm Issues and PRs related to the ShadowRealm API and node::Realm label Dec 26, 2022
legendecas added a commit that referenced this pull request Dec 28, 2022
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
legendecas added a commit that referenced this pull request Dec 28, 2022
Create worker binding templates in the per-isolate initialization.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@legendecas
Copy link
Member Author

Landed in 796c4f9...8dfd905

@legendecas legendecas closed this Dec 28, 2022
@legendecas legendecas deleted the shadow-realm/per-realm-binding branch December 28, 2022 04:20
vitpavlenko pushed a commit to vitpavlenko/node that referenced this pull request Dec 28, 2022
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: nodejs#45547
Refs: nodejs#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
vitpavlenko pushed a commit to vitpavlenko/node that referenced this pull request Dec 28, 2022
Create worker binding templates in the per-isolate initialization.

PR-URL: nodejs#45547
Refs: nodejs#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 1, 2023
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 1, 2023
Create worker binding templates in the per-isolate initialization.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
Create worker binding templates in the per-isolate initialization.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
Create worker binding templates in the per-isolate initialization.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jan 24, 2023
@juanarbol
Copy link
Member

This is not landing cleanly in the v18.x line 💔

@legendecas
Copy link
Member Author

Backport to v18.x depends on #46336.

legendecas added a commit to legendecas/node that referenced this pull request May 23, 2023
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: nodejs#45547
Refs: nodejs#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
legendecas added a commit to legendecas/node that referenced this pull request May 23, 2023
Create worker binding templates in the per-isolate initialization.

PR-URL: nodejs#45547
Refs: nodejs#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@legendecas
Copy link
Member Author

Backport to v18.x depends on #44488.

targos pushed a commit that referenced this pull request Nov 10, 2023
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: #45547
Refs: #42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: nodejs/node#45547
Refs: nodejs/node#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Bindings that need to be loaded in distinct contexts of node::Realm in
the same node::Environment instance needs to share the per-isolate
templates.

Add a new binding registration callback, which is called with
per-IsolateData. This allows bindings to define templates and
share them across realms eagerly, and avoid accidental modification on
the templates when the per-context callback is called multiple times.

This also tracks loaded bindings and built-in modules with node::Realm.

PR-URL: nodejs/node#45547
Refs: nodejs/node#42528
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. 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. realm Issues and PRs related to the ShadowRealm API and node::Realm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants