-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Revert realms from deno_core #16366
Revert realms from deno_core #16366
Conversation
… to `ContextState` (denoland#15599)" This reverts commit 307d84c.
…)" This reverts commit f16fe44.
Please add a PR description with reasoning. |
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
This revert has been discussed at length out-of-band (including with @andreubotella). The realms work in impeding ongoing event loop and performance work. We very much want to land realms but it needs to wait until these lower-level refactors are complete. We hope to bring realms back in a couple weeks.
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`, 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 removed in denoland#16366.
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.
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.
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.
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.
…#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.
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]>
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.
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]>
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]>
This revert has been discussed at length out-of-band (including with @andreubotella). The realms work in impeding ongoing event loop and performance work. We very much want to land realms but it needs to wait until these lower-level refactors are complete. We hope to bring realms back in a couple weeks.