fix deadlock in file.rs

compiler/load/src/file.rs

@@ -293,7 +293,7 @@ struct ModuleCache<'a> {
     sources: MutMap<ModuleId, (PathBuf, &'a str)>,
 }
 
-fn start_phase<'a>(module_id: ModuleId, phase: Phase, state: &mut State<'a>) -> BuildTask<'a> {
+fn start_phase<'a>(module_id: ModuleId, phase: Phase, state: &mut State<'a>) -> Vec<BuildTask<'a>> {
     // we blindly assume all dependencies are met
 
     match state.dependencies.status.get_mut(&(module_id, phase)) {
@@ -301,11 +301,20 @@ fn start_phase<'a>(module_id: ModuleId, phase: Phase, state: &mut State<'a>) ->
             // start this phase!
             *current = Status::Pending;
         }
-        Some(Status::Pending) | Some(Status::Done) => {
+        Some(Status::Pending) => {
             // don't start this task again!
-            todo!();
+            return vec![];
+        }
+        Some(Status::Done) => {
+            // don't start this task again, but tell those waiting for it they can continue
+            return state
+                .dependencies
+                .notify(module_id, phase)
+                .into_iter()
+                .map(|(module_id, phase)| start_phase(module_id, phase, state))
+                .flatten()
+                .collect();
         }
-
         None => match phase {
             Phase::LoadHeader => {
                 // this is fine, mark header loading as pending
@@ -321,78 +330,80 @@ fn start_phase<'a>(module_id: ModuleId, phase: Phase, state: &mut State<'a>) ->
         },
     }
 
-    match phase {
+    let task = {
-        Phase::LoadHeader => {
+        match phase {
-            let dep_name = state
+            Phase::LoadHeader => {
-                .module_cache
+                let dep_name = state
-                .module_names
+                    .module_cache
-                .remove(&module_id)
+                    .module_names
-                .expect("module id is present");
+                    .remove(&module_id)
+                    .expect("module id is present");
 
-            BuildTask::LoadModule {
+                BuildTask::LoadModule {
-                module_name: dep_name,
+                    module_name: dep_name,
-                // Provide mutexes of ModuleIds and IdentIds by module,
+                    // Provide mutexes of ModuleIds and IdentIds by module,
-                // so other modules can populate them as they load.
+                    // so other modules can populate them as they load.
-                module_ids: Arc::clone(&state.arc_modules),
+                    module_ids: Arc::clone(&state.arc_modules),
-                ident_ids_by_module: Arc::clone(&state.ident_ids_by_module),
+                    ident_ids_by_module: Arc::clone(&state.ident_ids_by_module),
-                mode: state.stdlib.mode,
+                    mode: state.stdlib.mode,
-            }
-        }
-        Phase::Parse => {
-            // parse the file
-            let header = state.module_cache.headers.remove(&module_id).unwrap();
-
-            BuildTask::Parse { header }
-        }
-        Phase::CanonicalizeAndConstrain => {
-            // canonicalize the file
-            let parsed = state.module_cache.parsed.remove(&module_id).unwrap();
-
-            let deps_by_name = &parsed.deps_by_name;
-            let num_deps = deps_by_name.len();
-            let mut dep_idents: MutMap<ModuleId, IdentIds> = IdentIds::exposed_builtins(num_deps);
-
-            let State {
-                ident_ids_by_module,
-                ..
-            } = &state;
-
-            {
-                let ident_ids_by_module = (*ident_ids_by_module).lock();
-
-                // Populate dep_idents with each of their IdentIds,
-                // which we'll need during canonicalization to translate
-                // identifier strings into IdentIds, which we need to build Symbols.
-                // We only include the modules we care about (the ones we import).
-                //
-                // At the end of this loop, dep_idents contains all the information to
-                // resolve a symbol from another module: if it's in here, that means
-                // we have both imported the module and the ident was exported by that mdoule.
-                for dep_id in deps_by_name.values() {
-                    // We already verified that these are all present,
-                    // so unwrapping should always succeed here.
-                    let idents = ident_ids_by_module.get(&dep_id).unwrap();
-
-                    dep_idents.insert(*dep_id, idents.clone());
                 }
             }
+            Phase::Parse => {
+                // parse the file
+                let header = state.module_cache.headers.remove(&module_id).unwrap();
 
-            // Clone the module_ids we'll need for canonicalization.
+                BuildTask::Parse { header }
-            // This should be small, and cloning it should be quick.
+            }
-            // We release the lock as soon as we're done cloning, so we don't have
+            Phase::CanonicalizeAndConstrain => {
-            // to lock the global module_ids while canonicalizing any given module.
+                // canonicalize the file
-            let module_ids = Arc::clone(&state.arc_modules);
+                let parsed = state.module_cache.parsed.remove(&module_id).unwrap();
-            let module_ids = { (*module_ids).lock().clone() };
 
-            let exposed_symbols = state
+                let deps_by_name = &parsed.deps_by_name;
-                .exposed_symbols_by_module
+                let num_deps = deps_by_name.len();
-                .remove(&module_id)
+                let mut dep_idents: MutMap<ModuleId, IdentIds> =
-                .expect("Could not find listener ID in exposed_symbols_by_module");
+                    IdentIds::exposed_builtins(num_deps);
 
-            let mut aliases = MutMap::default();
+                let State {
+                    ident_ids_by_module,
+                    ..
+                } = &state;
 
-            for imported in parsed.imported_modules.iter() {
+                {
-                match state.module_cache.aliases.get(imported) {
+                    let ident_ids_by_module = (*ident_ids_by_module).lock();
+
+                    // Populate dep_idents with each of their IdentIds,
+                    // which we'll need during canonicalization to translate
+                    // identifier strings into IdentIds, which we need to build Symbols.
+                    // We only include the modules we care about (the ones we import).
+                    //
+                    // At the end of this loop, dep_idents contains all the information to
+                    // resolve a symbol from another module: if it's in here, that means
+                    // we have both imported the module and the ident was exported by that mdoule.
+                    for dep_id in deps_by_name.values() {
+                        // We already verified that these are all present,
+                        // so unwrapping should always succeed here.
+                        let idents = ident_ids_by_module.get(&dep_id).unwrap();
+
+                        dep_idents.insert(*dep_id, idents.clone());
+                    }
+                }
+
+                // Clone the module_ids we'll need for canonicalization.
+                // This should be small, and cloning it should be quick.
+                // We release the lock as soon as we're done cloning, so we don't have
+                // to lock the global module_ids while canonicalizing any given module.
+                let module_ids = Arc::clone(&state.arc_modules);
+                let module_ids = { (*module_ids).lock().clone() };
+
+                let exposed_symbols = state
+                    .exposed_symbols_by_module
+                    .remove(&module_id)
+                    .expect("Could not find listener ID in exposed_symbols_by_module");
+
+                let mut aliases = MutMap::default();
+
+                for imported in parsed.imported_modules.iter() {
+                    match state.module_cache.aliases.get(imported) {
                     None => unreachable!(
                         "imported module {:?} did not register its aliases, so {:?} cannot use them",
                         imported,
@@ -403,99 +414,102 @@ fn start_phase<'a>(module_id: ModuleId, phase: Phase, state: &mut State<'a>) ->
                         aliases.extend(new.iter().map(|(s, a)| (*s, a.clone())));
                     }
                 }
+                }
+
+                BuildTask::CanonicalizeAndConstrain {
+                    parsed,
+                    dep_idents,
+                    exposed_symbols,
+                    module_ids,
+                    mode: state.stdlib.mode,
+                    aliases,
+                }
             }
 
-            BuildTask::CanonicalizeAndConstrain {
+            Phase::SolveTypes => {
-                parsed,
+                let constrained = state.module_cache.constrained.remove(&module_id).unwrap();
-                dep_idents,
+
-                exposed_symbols,
+                let ConstrainedModule {
-                module_ids,
+                    module,
-                mode: state.stdlib.mode,
+                    ident_ids,
-                aliases,
+                    module_timing,
+                    constraint,
+                    var_store,
+                    imported_modules,
+                    declarations,
+                    ..
+                } = constrained;
+
+                BuildTask::solve_module(
+                    module,
+                    ident_ids,
+                    module_timing,
+                    constraint,
+                    var_store,
+                    imported_modules,
+                    &mut state.exposed_types,
+                    &state.stdlib,
+                    declarations,
+                )
+            }
+            Phase::FindSpecializations => {
+                let typechecked = state.module_cache.typechecked.remove(&module_id).unwrap();
+
+                let TypeCheckedModule {
+                    layout_cache,
+                    module_id,
+                    module_timing,
+                    solved_subs,
+                    decls,
+                    ident_ids,
+                } = typechecked;
+
+                BuildTask::BuildPendingSpecializations {
+                    layout_cache,
+                    module_id,
+                    module_timing,
+                    solved_subs,
+                    decls,
+                    ident_ids,
+                    exposed_to_host: state.exposed_to_host.clone(),
+                }
+            }
+            Phase::MakeSpecializations => {
+                let found_specializations = state
+                    .module_cache
+                    .found_specializations
+                    .remove(&module_id)
+                    .unwrap();
+
+                let specializations_we_must_make = state
+                    .module_cache
+                    .external_specializations_requested
+                    .remove(&module_id)
+                    .unwrap_or_default();
+
+                let FoundSpecializationsModule {
+                    module_id,
+                    ident_ids,
+                    subs,
+                    procs,
+                    layout_cache,
+                    module_timing,
+                } = found_specializations;
+
+                BuildTask::MakeSpecializations {
+                    module_id,
+                    ident_ids,
+                    subs,
+                    procs,
+                    layout_cache,
+                    specializations_we_must_make,
+                    module_timing,
+                }
             }
         }
+    };
 
-        Phase::SolveTypes => {
+    vec![task]
-            let constrained = state.module_cache.constrained.remove(&module_id).unwrap();
-
-            let ConstrainedModule {
-                module,
-                ident_ids,
-                module_timing,
-                constraint,
-                var_store,
-                imported_modules,
-                declarations,
-                ..
-            } = constrained;
-
-            BuildTask::solve_module(
-                module,
-                ident_ids,
-                module_timing,
-                constraint,
-                var_store,
-                imported_modules,
-                &mut state.exposed_types,
-                &state.stdlib,
-                declarations,
-            )
-        }
-        Phase::FindSpecializations => {
-            let typechecked = state.module_cache.typechecked.remove(&module_id).unwrap();
-
-            let TypeCheckedModule {
-                layout_cache,
-                module_id,
-                module_timing,
-                solved_subs,
-                decls,
-                ident_ids,
-            } = typechecked;
-
-            BuildTask::BuildPendingSpecializations {
-                layout_cache,
-                module_id,
-                module_timing,
-                solved_subs,
-                decls,
-                ident_ids,
-                exposed_to_host: state.exposed_to_host.clone(),
-            }
-        }
-        Phase::MakeSpecializations => {
-            let found_specializations = state
-                .module_cache
-                .found_specializations
-                .remove(&module_id)
-                .unwrap();
-
-            let specializations_we_must_make = state
-                .module_cache
-                .external_specializations_requested
-                .remove(&module_id)
-                .unwrap_or_default();
-
-            let FoundSpecializationsModule {
-                module_id,
-                ident_ids,
-                subs,
-                procs,
-                layout_cache,
-                module_timing,
-            } = found_specializations;
-
-            BuildTask::MakeSpecializations {
-                module_id,
-                ident_ids,
-                subs,
-                procs,
-                layout_cache,
-                specializations_we_must_make,
-                module_timing,
-            }
-        }
-    }
 }
 
 #[derive(Debug)]
@@ -1329,6 +1343,21 @@ where
     .unwrap()
 }
 
+fn start_tasks<'a>(
+    work: MutSet<(ModuleId, Phase)>,
+    state: &mut State<'a>,
+    injector: &Injector<BuildTask<'a>>,
+    worker_listeners: &'a [Sender<WorkerMsg>],
+) -> Result<(), LoadingProblem> {
+    for (module_id, phase) in work {
+        for task in start_phase(module_id, phase, state) {
+            enqueue_task(&injector, worker_listeners, task)?
+        }
+    }
+
+    Ok(())
+}
+
 fn update<'a>(
     mut state: State<'a>,
     msg: Msg<'a>,
@@ -1384,19 +1413,11 @@ fn update<'a>(
 
             state.module_cache.headers.insert(header.module_id, header);
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
-
-                enqueue_task(&injector, worker_listeners, task)?
-            }
 
             let work = state.dependencies.notify(home, Phase::LoadHeader);
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
-
-                enqueue_task(&injector, worker_listeners, task)?
-            }
 
             Ok(state)
         }
@@ -1412,11 +1433,7 @@ fn update<'a>(
 
             let work = state.dependencies.notify(module_id, Phase::Parse);
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
-
-                enqueue_task(&injector, worker_listeners, task)?
-            }
 
             Ok(state)
         }
@@ -1452,11 +1469,7 @@ fn update<'a>(
                 .dependencies
                 .notify(module_id, Phase::CanonicalizeAndConstrain);
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
-
-                enqueue_task(&injector, worker_listeners, task)?
-            }
 
             Ok(state)
         }
@@ -1503,11 +1516,7 @@ fn update<'a>(
                     .notify(module_id, Phase::CanonicalizeAndConstrain),
             );
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
-
-                enqueue_task(&injector, worker_listeners, task)?
-            }
 
             Ok(state)
         }
@@ -1594,11 +1603,7 @@ fn update<'a>(
                     state.constrained_ident_ids.insert(module_id, ident_ids);
                 }
 
-                for (module_id, phase) in work {
+                start_tasks(work, &mut state, &injector, worker_listeners)?;
-                    let task = start_phase(module_id, phase, &mut state);
-
-                    enqueue_task(&injector, worker_listeners, task)?
-                }
             }
 
             Ok(state)
@@ -1646,11 +1651,8 @@ fn update<'a>(
                 .dependencies
                 .notify(module_id, Phase::FindSpecializations);
 
-            for (module_id, phase) in work {
+            start_tasks(work, &mut state, &injector, worker_listeners)?;
-                let task = start_phase(module_id, phase, &mut state);
 
-                enqueue_task(&injector, worker_listeners, task)?
-            }
             Ok(state)
         }
         MadeSpecializations {
@@ -1718,11 +1720,7 @@ fn update<'a>(
                 // the originally requested module, we're all done!
                 return Ok(state);
             } else {
-                for (module_id, phase) in work {
+                start_tasks(work, &mut state, &injector, worker_listeners)?;
-                    let task = start_phase(module_id, phase, &mut state);
-
-                    enqueue_task(&injector, worker_listeners, task)?
-                }
             }
 
             Ok(state)