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.

Sign up for GitHub

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

Jump to bottom

fix(core): Have custom errors be created in the right realm #17050

Merged
merged 2 commits into from
Dec 20, 2022
Merged

fix(core): Have custom errors be created in the right realm #17050

merged 2 commits into from
Dec 20, 2022

Conversation

andreubotella
Copy link
Contributor

@andreubotella andreubotella commented Dec 14, 2022 •
edited
Loading

Although PR #16366 did not fully revert deno_core's support for realms, since JsRealm still existed after that, it did remove the ContextState struct, originally introduced in #14734. This change made js_build_custom_error_cb, among other properties, a per-runtime callback, rather than per-realm, which cause a bug where errors thrown from an op would always be constructed in the main realm, rather than in the current one.

This change adds back ContextState to fix this bug, adds back the known_realms field of JsRuntimeState (needed to be able to drop the callback when snapshotting), and also relands #14750, which adds the js_realm_sync_ops test for this bug that was removed in #16366.

@bartlomieju
Copy link
Member

@andreubotella please rebase

@andreubotella
fix(core): Have custom errors be created in the right realm
fd7928d 
Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change
made `js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than
in the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop
the callback when snapshotting), and also relands denoland#14750, which adds
the `js_realm_sync_ops` test for this bug that was removed in denoland#16366.
bartlomieju
bartlomieju reviewed Dec 18, 2022
View reviewed changes
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.

@littledivy are you fine with these changes?

littledivy
littledivy approved these changes Dec 18, 2022
View reviewed changes
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM.

@bartlomieju Yes, it isn't in the hot path. Let's watch for regressions anyways

bartlomieju
bartlomieju approved these changes Dec 20, 2022
View reviewed changes
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, thanks Andreu

@bartlomieju bartlomieju merged commit fcca54d into denoland:main Dec 20, 2022
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Dec 22, 2022
@andreubotella @bartlomieju
fix(core): Have custom errors be created in the right realm (denoland…
4bb8925 
…#17050)

Although PR denoland#16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in denoland#14734. This change made
`js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than in
the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop the
callback when snapshotting), and also relands denoland#14750, which adds the
`js_realm_sync_ops` test for this bug that was removed in denoland#16366.
@andreubotella andreubotella deleted the realms-fix-errors branch December 22, 2022 08:16
This was referenced Dec 28, 2022
Open
Merged
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 28, 2022
@andreubotella @lmalheiro
feat(core): Reland support for async ops in realms
ea66f19 
Currently realms are supported on `deno_core`, but there was no
support for async ops anywhere other than the main realm. The main
issue is that the `js_recv_cb` callback, which resolves promises
corresponding to async ops, was only set for the main realm, so async
ops in other realms would never resolve. Furthermore, promise ID's are
specific to each realm, which meant that async ops from other realms
would result in a wrong promise from the main realm being resolved.

This change takes the `ContextState` struct added in denoland#17050, and adds
to it a `js_recv_cb` callback for each realm. Combined with the fact
that that same PR also added a list of known realms to
`JsRuntimeState`, and that denoland#17174 made `OpCtx` instances
realm-specific and had them include an index into that list of known
realms, this makes it possible to know the current realm in the
`queue_async_op` and `queue_fast_async_op` methods, and therefore to
send the results of promises for each realm to that realm, and prevent
the ID's from getting mixed up.

Additionally, since promise ID's are no longer unique to the isolate,
having a single set of unrefed ops doesn't work. This change therefore
also moves `unrefed_ops` from `JsRuntimeState` to `ContextState`, and
adds the lengths of the unrefed op sets for all known realms to get
the total number of unrefed ops to compare in the event loop.

This PR is a reland of denoland#14734 after it was reverted in denoland#16366, except
that `ContextState` and `JsRuntimeState::known_realms` were previously
relanded in denoland#17050. Another significant difference with the original
PR is passing around an index into `JsRuntimeState::known_realms`
instead of a `v8::Global<v8::Context>` to identify the realm, because
async op queuing in fast calls cannot call into V8, and therefore
cannot have access to V8 globals. This also simplified the
implementation of `resolve_async_ops`.

Co-authored-by: Luis Malheiro <[email protected]>
bartlomieju pushed a commit that referenced this pull request Jan 5, 2023
@andreubotella @bartlomieju
fix(core): Have custom errors be created in the right realm (#17050)
175f973 
Although PR #16366 did not fully revert `deno_core`'s support for
realms, since `JsRealm` still existed after that, it did remove the
`ContextState` struct, originally introduced in #14734. This change made
`js_build_custom_error_cb`, among other properties, a per-runtime
callback, rather than per-realm, which cause a bug where errors thrown
from an op would always be constructed in the main realm, rather than in
the current one.

This change adds back `ContextState` to fix this bug, adds back the
`known_realms` field of `JsRuntimeState` (needed to be able to drop the
callback when snapshotting), and also relands #14750, which adds the
`js_realm_sync_ops` test for this bug that was removed in #16366.
bartlomieju pushed a commit that referenced this pull request Jan 14, 2023
@andreubotella @lmalheiro
feat(core): Reland support for async ops in realms (#17204)
6878234 
Currently realms are supported on `deno_core`, but there was no support
for async ops anywhere other than the main realm. The main issue is that
the `js_recv_cb` callback, which resolves promises corresponding to
async ops, was only set for the main realm, so async ops in other realms
would never resolve. Furthermore, promise ID's are specific to each
realm, which meant that async ops from other realms would result in a
wrong promise from the main realm being resolved.

This change takes the `ContextState` struct added in #17050, and adds to
it a `js_recv_cb` callback for each realm. Combined with the fact that
that same PR also added a list of known realms to `JsRuntimeState`, and
that #17174 made `OpCtx` instances realm-specific and had them include
an index into that list of known realms, this makes it possible to know
the current realm in the `queue_async_op` and `queue_fast_async_op`
methods, and therefore to send the results of promises for each realm to
that realm, and prevent the ID's from getting mixed up.

Additionally, since promise ID's are no longer unique to the isolate,
having a single set of unrefed ops doesn't work. This change therefore
also moves `unrefed_ops` from `JsRuntimeState` to `ContextState`, and
adds the lengths of the unrefed op sets for all known realms to get the
total number of unrefed ops to compare in the event loop.

This PR is a reland of #14734 after it was reverted in #16366, except
that `ContextState` and `JsRuntimeState::known_realms` were previously
relanded in #17050. Another significant difference with the original PR
is passing around an index into `JsRuntimeState::known_realms` instead
of a `v8::Global<v8::Context>` to identify the realm, because async op
queuing in fast calls cannot call into V8, and therefore cannot have
access to V8 globals. This also simplified the implementation of
`resolve_async_ops`.

Co-authored-by: Luis Malheiro <[email protected]>
bartlomieju pushed a commit that referenced this pull request Jan 16, 2023
@andreubotella @lmalheiro @bartlomieju
feat(core): Reland support for async ops in realms (#17204)
4bd2960 
Currently realms are supported on `deno_core`, but there was no support
for async ops anywhere other than the main realm. The main issue is that
the `js_recv_cb` callback, which resolves promises corresponding to
async ops, was only set for the main realm, so async ops in other realms
would never resolve. Furthermore, promise ID's are specific to each
realm, which meant that async ops from other realms would result in a
wrong promise from the main realm being resolved.

This change takes the `ContextState` struct added in #17050, and adds to
it a `js_recv_cb` callback for each realm. Combined with the fact that
that same PR also added a list of known realms to `JsRuntimeState`, and
that #17174 made `OpCtx` instances realm-specific and had them include
an index into that list of known realms, this makes it possible to know
the current realm in the `queue_async_op` and `queue_fast_async_op`
methods, and therefore to send the results of promises for each realm to
that realm, and prevent the ID's from getting mixed up.

Additionally, since promise ID's are no longer unique to the isolate,
having a single set of unrefed ops doesn't work. This change therefore
also moves `unrefed_ops` from `JsRuntimeState` to `ContextState`, and
adds the lengths of the unrefed op sets for all known realms to get the
total number of unrefed ops to compare in the event loop.

This PR is a reland of #14734 after it was reverted in #16366, except
that `ContextState` and `JsRuntimeState::known_realms` were previously
relanded in #17050. Another significant difference with the original PR
is passing around an index into `JsRuntimeState::known_realms` instead
of a `v8::Global<v8::Context>` to identify the realm, because async op
queuing in fast calls cannot call into V8, and therefore cannot have
access to V8 globals. This also simplified the implementation of
`resolve_async_ops`.

Co-authored-by: Luis Malheiro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

@littledivy littledivy littledivy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andreubotella @bartlomieju @littledivy