Skip to content

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 force-pushed the shadow-realm/per-realm-binding branch from d115a82 to c24d797 Compare November 22, 2022 07:43
@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
@legendecas legendecas force-pushed the shadow-realm/per-realm-binding branch from bbecbcd to 6311182 Compare November 22, 2022 17:42
@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
@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

@legendecas
Copy link
Member Author

@nodejs/realm PTAL, thank you!

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 force-pushed the shadow-realm/per-realm-binding branch from 0dc40ad to f264321 Compare December 19, 2022 18:02
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. 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.

6 participants