-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Add deno::RecursiveLoad for async module loading #2034
Conversation
core/modules.rs
Outdated
/// an Isolate during load. | ||
pub struct RecursiveLoad<E: Error, B: Behavior, L: Loader<E, B>> { | ||
loader: Option<L>, | ||
pending: Vec<PendingLoad<E>>, |
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.
If you make this a HashMap<String, Box> you can use it do avoid loading the same url twice in parallel.
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.
I would still recommend this.
core/modules.rs
Outdated
|
||
// TODO Fix this resolve callback weirdness. | ||
let loader_ = | ||
unsafe { std::mem::transmute::<&mut L, &'static mut L>(&mut loader) }; |
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.
;-(
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.
There are two problems here.
First is that isolate::ResolveFn is defined as FnMut(...) -> deno_mod + 'static
. The 'static part is implicit. This is easy to fix.
The other problem is that if you mutably borrow the isolate
field from MockLoader, the loader is locked thereafter and it's other fields are inaccessible. So the following pattern can't possibly work:
loader.use_isolate(|isolate| {
isolate.mod_instantiate(some_id, |specifier, referrer| {
let id = loader.resolve(specifier, referred); // <-- This doesn't work because `loader.use_isolate()` has made `loader` inaccessible.
// ... do something with id.
});
});
This is more difficult to fix and speaks against the design with use_modules()/use_isolate().
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.
Partially fixed by merging use_isolate()
and use_modules()
into one function fn enter<R, F: FnMut(&mut Isolate<Self::Behavior>, &mut Modules) -> R>(&mut self, cb: F) -> R;
and making resolve()
a static method.
Whether this is sufficient when doing the 'real' Loader implementation remains to be seen; it could be that you'll add more fields that you'll want to access while also having access to the isolate/modules.
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.
I've already done the real loader. I'm just breaking this up for ease of landing.
See #2002
394ad19
to
5f9c8e0
Compare
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
Seeing a failure in CI on linux/gn/release:
I can't repeat but here's the command to try:
|
709500c
to
3cb6d52
Compare
3cb6d52
to
917e68f
Compare
Rename IsolateState to ThreadSafeState Make ThreadSafeState directly implement Dispatch. This is simpler. Revert "Refactor deno_core::RecursiveLoad to be more idiomatic (denoland#2034)" This reverts commit 917e68f. integration2 WIP Reorganize print_file_info code fix resolve_module2 Further disable some tests tests pass fix another test Add worker::tests::execute_mod_resolve_error
No description provided.