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

refactor(core): Move optional callbacks from JsRuntimeState to ContextState #15599

Merged

Conversation

andreubotella
Copy link
Contributor

@andreubotella andreubotella commented Aug 25, 2022

The JsRuntimeState struct stores a number of JS callbacks that are used either in the event loop or when interacting with V8. Some of these callback fields are vectors of callbacks, and therefore could plausibly store at least one callback per realm. However, some of those fields are Option<v8::Global<v8::Function>>, which would make the callbacks set by a realm override the one that might have been set by a different realm.

As it turns out, all of the current such optional callbacks (js_promise_reject_cb, js_format_exception_cb and js_wasm_streaming_cb) are only used from inside a realm, and therefore this change makes it so such callbacks can only be set from inside a realm, and will only affect that realm.

Towards #13239.

…textState`

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of
these callback fields are vectors of callbacks, and therefore could
plausibly store at least one callback per realm. However, some of
those fields are `Option<v8::Global<v8::Function>>`, which would make
the callbacks set by a realm override the one that might have been set
by a different realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and
therefore this change makes it so such callbacks can only be set from
inside a realm, and will only affect that realm.

Towards denoland#13239.
@andreubotella andreubotella marked this pull request as ready for review August 25, 2022 18:05
@andreubotella
Copy link
Contributor Author

Most of these callbacks aren't really tested in unit tests but in the cli integration tests, and we don't currently have any way to run realm tests that aren't driven by Rust. Is it fine to just have this single test covering js_promise_reject_cb? Or should I write unit tests for the other callbacks?

@bartlomieju
Copy link
Member

Most of these callbacks aren't really tested in unit tests but in the cli integration tests, and we don't currently have any way to run realm tests that aren't driven by Rust. Is it fine to just have this single test covering js_promise_reject_cb? Or should I write unit tests for the other callbacks?

Yes, I think that's fine for now

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @andreubotella

@bartlomieju bartlomieju merged commit 307d84c into denoland:main Sep 1, 2022
@andreubotella andreubotella deleted the move-callbacks-to-context-state branch September 1, 2022 23:05
littledivy added a commit to littledivy/deno that referenced this pull request Oct 20, 2022
andreubotella added a commit to andreubotella/deno that referenced this pull request Jan 14, 2023
…textState`

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of
these callback fields are vectors of callbacks, and therefore could
plausibly store at least one callback per realm. However, some of
those fields are `Option<v8::Global<v8::Function>>`, which would make
the callbacks set by a realm override the one that might have been set
by a different realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and
therefore this change makes it so such callbacks can only be set from
inside a realm, and will only affect that realm.

This is a reland of denoland#15599.

Towards denoland#13239.
littledivy pushed a commit that referenced this pull request Jan 15, 2023
…textState` (#17422)

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of these
callback fields are vectors of callbacks, and therefore could plausibly
store at least one callback per realm. However, some of those fields are
`Option<v8::Global<v8::Function>>`, which would make the callbacks set
by a realm override the one that might have been set by a different
realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and therefore
this change makes it so such callbacks can only be set from inside a
realm, and will only affect that realm.

This is a reland of #15599.

Towards #13239.
bartlomieju pushed a commit that referenced this pull request Jan 16, 2023
…textState` (#17422)

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of these
callback fields are vectors of callbacks, and therefore could plausibly
store at least one callback per realm. However, some of those fields are
`Option<v8::Global<v8::Function>>`, which would make the callbacks set
by a realm override the one that might have been set by a different
realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and therefore
this change makes it so such callbacks can only be set from inside a
realm, and will only affect that realm.

This is a reland of #15599.

Towards #13239.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants