fix(core/modules): Fix concurrent loading of dynamic imports #11089
Conversation
There was a problem hiding this comment.
@nayeemrmn great to see you looked into it. I've got some reservations that even though it fixes provided test case it might not be perfect solution.
Could you please use test case from #8421 (the unit test case to put in deno_core instead of integration one). I'm afraid we might have multiple calls to load() for the same module (would be good to assert number of the loads).
I guess this is a good way forward but ultimately we should keep track of all visited modules in ModuleMap instead of RecursiveModuleLoad.
nayeemrmn
force-pushed
the
concurrent-dynamic-imports
branch
from
6e4a1e5
to
05b6f46
Compare
nayeemrmn
force-pushed
the
concurrent-dynamic-imports
branch
from
f511032
to
87a224e
Compare
There was a problem hiding this comment.
Looks good to me 🚀
Thank you Nayeem, this problem has been haunting me for the past half a year. This is a very elegant solution to the problem, and it didn't require rewriting all of the module loading subsystem. Cleanly implemented with plenty of comments. Cheers!
| init: LoadInit, | ||
| // TODO(bartlomieju): in future this value should | ||
| // be randomized | ||
| pub id: ModuleLoadId, | ||
| pub root_module_id: Option<ModuleId>, | ||
| pub state: LoadState, | ||
| pub module_map_rc: Rc<RefCell<ModuleMap>>, |
| // Recurse the module's imports. There are two cases for each import: | ||
| // 1. If the module is not in the module map, start a new load for it in | ||
| // `self.pending`. The result of that load should eventually be passed to | ||
| // this function for recursion. | ||
| // 2. If the module is already in the module map, queue it up to be | ||
| // recursed synchronously here. | ||
| // This robustly ensures that the whole graph is in the module map before | ||
| // `LoadState::Done` is set. | ||
| let specifier = | ||
| crate::resolve_url(&module_source.module_url_found).unwrap(); | ||
| let mut already_registered = VecDeque::new(); | ||
| already_registered.push_back((module_id, specifier.clone())); | ||
| self.visited.insert(specifier); | ||
| while let Some((module_id, referrer)) = already_registered.pop_front() { |
There was a problem hiding this comment.
Nice! I believe this is the meat of this PR, very elegantly solved 👍
| // TODO(nayeemrmn): In this case we would ideally skip to | ||
| // `LoadState::LoadingImports` and synchronously recurse the imports | ||
| // like the bottom of `RecursiveModuleLoad::register_and_recurse()`. | ||
| // But the module map cannot be borrowed here. Instead fake a load | ||
| // event so it gets passed to that function and recursed eventually. |
There was a problem hiding this comment.
We can address this bit when adding support for import assertions
| // Regression test for https://github.com/denoland/deno/issues/3736. | ||
| #[test] | ||
| fn dyn_concurrent_circular_import() { |
This commit changes implementation of module loading in "deno_core" to track all currently fetched modules across all existing module loads. In effect a bug that caused concurrent dynamic imports referencing the same module to fail is fixed.
Fixes #3736.
The following guard:
deno/core/modules.rs
Line 583 in 4772730
RecursiveModuleLoadto not recurse a modulefoo's dependencies if it is already in the module map, causing the load to complete early, and letting the runtime try to instantiatefoo. But if there is another pending load offoo, it may be the case thatfoois in the module map but its dependencies aren't yet, which will cause instantiation to fail. So we should always visit all dependencies in a load and check registration for each module instead.cc @bartlomieju