refactor(core): unwrap sync ops in rust #15449
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull request denoland#14019 enabled initial support for realms, but it did not include support for async ops anywhere other than the main realm. The main issue was 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 creates a `ContextState` struct, similar to `JsRuntimeState` but stored in a slot of each `v8::Context`, which contains a `js_recv_cb` callback for each realm. Combined with a new list of known realms, which stores them as `v8::Weak<v8::Context>`, and a change in the `#[op]` macro to pass the current context to `queue_async_op`, this makes it possible to send the results of promises for different realms to their 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. Towards denoland#13239. Co-authored-by: Luis Malheiro <[email protected]>
This is so the `ContextState` reference isn't tied to the lifetime of the isolate. The `JsRealm::state_from_scope` method used to take a callback and pass it a `&mut ContextState` because the mutable reference was tied to the lifetime of a variable internal to the function, but with this change it isn't anymore. So this method is also updated to return a `Rc<RefCell<ContextState>>`.
…ise-ring' into op-sync-unwrap-in-rust
This reverts commit 4327a6d.
…ise-ring' into op-sync-unwrap-in-rust
ghost
reviewed
Aug 11, 2022
| } | ||
| return error; | ||
| } | ||
|
|
||
| function unwrapOpResult(res) { |
There was a problem hiding this comment.
You should now be able to remove the unwrapOpResult function, yes?
There was a problem hiding this comment.
It's still used for async ops
|
LGTM |
aapoalas
reviewed
Aug 11, 2022
ghost
reviewed
Aug 11, 2022
Comment on lines
+131
to
+139
| const error = errorMap[className]?.(message); | ||
| // Strip buildCustomError() calls from stack trace | ||
| if (typeof error == "object") { | ||
| ErrorCaptureStackTrace(error, buildCustomError); | ||
| if (code) { | ||
| error.code = code; | ||
| } | ||
| } | ||
| return error; |
There was a problem hiding this comment.
Nit: minor, but it can be clearer, (when isn't the result an object?)
Suggested change
| const error = errorMap[className]?.(message); | |
| // Strip buildCustomError() calls from stack trace | |
| if (typeof error == "object") { | |
| ErrorCaptureStackTrace(error, buildCustomError); | |
| if (code) { | |
| error.code = code; | |
| } | |
| } | |
| return error; | |
| const errorConstructor = errorMap[className]; | |
| if (errorConstructor !== undefined) { | |
| const error = errorConstructor(message); | |
| // Strip buildCustomError() calls from stack trace | |
| ErrorCaptureStackTrace(error, buildCustomError); | |
| if (code) { | |
| error.code = code; | |
| } | |
| return error; | |
| } |
There was a problem hiding this comment.
Theoretically the error constructor function could return anything, eg. null, I guess.
aapoalas
approved these changes
Aug 11, 2022
bartlomieju
pushed a commit
to littledivy/deno
that referenced
this pull request
Aug 11, 2022
dsherret
pushed a commit
that referenced
this pull request
Aug 11, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Helps with #15337.
Supersedes #14811.
I messed up the branch at #15351 and had to open a new PR.