refactor: add 'setup' module, move isolate creation code #499
Conversation
| // TODO(bartlomieju): `deno_core_ext` ops should be returned as a separate | ||
| // vector - they need to be special cased and attached to `Deno.core.ops`, |
bartlomieju
force-pushed
the
cleanup_jsruntime_creation
branch
from
2758d91
to
d311605
Compare
bartlomieju
force-pushed
the
cleanup_jsruntime_creation
branch
from
b063159
to
7bffe03
Compare
core/runtime/bindings.rs
Outdated
| @@ -89,6 +89,9 @@ pub(crate) fn external_references( | |||
| let refs = v8::ExternalReferences::new(&references); | |||
| // Leak, V8 takes ownership of the references. | |||
| std::mem::forget(references); | |||
|
|
|||
| // V8 takes ownership of external_references. | |||
There was a problem hiding this comment.
This is actually something we need to fix eventually. It should be dropped along with the runtime.
| additional_references | ||
| .extend_from_slice(extension.get_external_references()); | ||
| } | ||
| let mut op_state = OpState::new(options.feature_checker.take()); |
There was a problem hiding this comment.
For later: we should move a lot of this opstate setup into the core extension itself. I started adding the task spawners there as well.
| .expect("Failed to evaluate extension JS"); | ||
|
|
||
| // TODO(bartlomieju): clean this up - we shouldn't need to store extensions | ||
| // on the `js_runtime` as they are mutable. | ||
| js_runtime.extensions = extensions; |
There was a problem hiding this comment.
I think this was previously done for realms support. It might not be required any more.
There was a problem hiding this comment.
This is only required because of snapshot_util using it to print Cargo directives. I opened #502.
| ) | ||
| .unwrap(); | ||
| module_map.instantiate_module(scope, mod_id).unwrap(); | ||
| let mut receiver = module_map.mod_evaluate(scope, mod_id); |
There was a problem hiding this comment.
Can we move this to a mod_evaluate_sync method in ModuleMap? Polling an async function with a noop-waker is not a great pattern as it should require extra scrutiny every time we see it. The extension sync loader code could mostly be hoisted into the ModuleMap as mod_evaluate_sync:
match receiver.poll_unpin(&mut Context::from_waker(noop_waker_ref())) {
Poll::Ready(result) => {
result
.with_context(|| format!("Couldn't execute '{specifier}'"))?;
}
Poll::Pending => {
// Find the TLA location and return it as an error
let scope = &mut realm.handle_scope(self.v8_isolate());
let messages = module_map.find_stalled_top_level_await(scope);
assert!(!messages.is_empty());
let msg = v8::Local::new(scope, &messages[0]);
let js_error = JsError::from_v8_message(scope, msg);
return Err(Error::from(js_error))
.with_context(|| "Top-level await is not allowed in extensions");
}
}
There was a problem hiding this comment.
Sure, I will do it in a follow up PR.
This commit removes entanglement between "OpCtx" and "ContextState" structs. "OpCtx" no longer requires "ContextState" to be passed as an argument. Instead it accepts "op_driver" arg which is "OpDriverImpl" (renamed from "DefaultOpDriver"). This allows us to fully set up "OpCtx" for every op and only then create "ContextState". This unlocks a lock of further clean ups that are done in #499.
This commit add "deno_core::runtime::setup" module and factors
out some non-trivial pieces of logic into standalone functions.
A lot of this code has been organically added over the years and it
becomes harder and harder to reason about op registration as well
as
OpStatesetup.In preparation for #489
Towards #52