diff --git a/app/shell/panels.js b/app/shell/panels.js index cbe68596..9bcc4d07 100644 --- a/app/shell/panels.js +++ b/app/shell/panels.js @@ -36,6 +36,7 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) let ui = null; if (spec.ui) ui = await load(spec.ui); + let teardown = null; const panel = renderPanel(spec, { values: values.valuesOf(spec.id), @@ -46,7 +47,9 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) host.appendChild(panel.node); if (ui && typeof ui.setup === "function") { - ui.setup({ + // Kept, not discarded: setup() returns the unsubscribe from on(), and + // dropping it means every re-mount leaves another live listener behind. + teardown = ui.setup({ composer, values: { get: () => values.valuesOf(spec.id), @@ -65,7 +68,14 @@ export async function mountAll(manifest, { load = (path) => import(path) } = {}) }); } - mounted.push({ id: spec.id, panel }); + mounted.push({ + id: spec.id, + panel, + destroy() { + if (typeof teardown === "function") teardown(); + panel.destroy(); + }, + }); } catch (err) { hidden.push({ id: spec.id, why: `${err.name}: ${err.message}` }); console.warn(`[FunPack] ${spec.id} did not mount: ${err.message}`); diff --git a/core/relations.py b/core/relations.py index f0e7ef8d..5d5ce1ff 100644 --- a/core/relations.py +++ b/core/relations.py @@ -6,8 +6,9 @@ about each other. A relation is local -- a module names only the neighbours it actually cares about, and everything else sorts itself. """ -from typing import Dict, Iterable, List, Tuple +from typing import Dict, Iterable, List, Set, Tuple +from . import log from .contract import STAGES, ModuleSpec @@ -26,22 +27,35 @@ def order(specs: Iterable[ModuleSpec]) -> Tuple[List[ModuleSpec], List[Tuple[Mod by_id = {spec.id: spec for spec in specs} rejected: List[Tuple[ModuleSpec, str]] = [] + stage_of = {spec.id: STAGES.index(spec.stage) for spec in specs} + # Edges point from "runs earlier" to "runs later". `before` is expressed as # the same edge from the other end, so the two spellings cannot disagree. edges: Dict[str, set] = {spec.id: set() for spec in specs} + + def _link(later: str, earlier: str) -> None: + # Stage is the coarse order and a relation may not overturn it: a load + # module cannot meaningfully run after a conditioning one, and honouring + # that would put a whole stage out of sequence to satisfy one module. + # The relation is dropped and said out loud rather than silently obeyed. + if stage_of[later] < stage_of[earlier]: + log.note( + f"ignoring {later!r} after {earlier!r}: {later!r} is in stage " + f"{STAGES[stage_of[later]]!r}, which runs before {STAGES[stage_of[earlier]]!r}" + ) + return + edges[later].add(earlier) + for spec in specs: + # A relation naming an absent module is not an error: modules come and + # go, and "after audio_clock" has nothing to wait for when audio_clock + # failed to load or was never installed. for earlier in spec.after: if earlier in by_id: - edges[spec.id].add(earlier) + _link(spec.id, earlier) for later in spec.before: if later in by_id: - edges[later].add(spec.id) - - # A relation naming an absent module is not an error: modules come and go, - # and "after audio_clock" simply has nothing to wait for when audio_clock - # failed to load or was never installed. - - stage_of = {spec.id: STAGES.index(spec.stage) for spec in specs} + _link(later, spec.id) ordered: List[ModuleSpec] = [] placed: set = set() @@ -52,10 +66,28 @@ def order(specs: Iterable[ModuleSpec]) -> Tuple[List[ModuleSpec], List[Tuple[Mod while remaining: ready = [s for s in remaining if edges[s.id] <= placed] if not ready: - for spec in remaining: - unmet = sorted(edges[spec.id] - placed) - rejected.append((spec, f"ordering cycle with {', '.join(unmet)}")) - break + # Drop only the modules actually ON a cycle. Dropping everything + # still waiting takes healthy modules with it -- one that merely + # says "after B" is not itself circular, and once B is gone its + # relation simply has nothing to wait for. + live = {s.id for s in remaining} + looping = _on_a_cycle(edges, live) + for spec in [s for s in remaining if s.id in looping]: + others = sorted(edges[spec.id] & looping) + rejected.append((spec, f"ordering cycle with {', '.join(others)}")) + remaining.remove(spec) + # A dropped module stops being something to wait for. Otherwise + # everything downstream of the cycle waits on an id that will never + # arrive, and the collateral damage spreads instead of stopping. + for waiting in edges.values(): + waiting -= looping + if not looping: + # Nothing is circular and nothing is ready: unreachable, but + # looping forever would be worse than saying so. + for spec in remaining: + rejected.append((spec, "could not be ordered")) + break + continue # Among the ready, respect stage so a later stage never jumps ahead. nxt = min(ready, key=lambda s: (stage_of[s.id], s.id)) ordered.append(nxt) @@ -63,3 +95,21 @@ def order(specs: Iterable[ModuleSpec]) -> Tuple[List[ModuleSpec], List[Tuple[Mod remaining.remove(nxt) return ordered, rejected + + +def _on_a_cycle(edges: Dict[str, set], live: Set[str]) -> Set[str]: + """Ids that can reach themselves through the edges still in play.""" + on_cycle: Set[str] = set() + for start in live: + seen: Set[str] = set() + stack = [n for n in edges[start] if n in live] + while stack: + node = stack.pop() + if node == start: + on_cycle.add(start) + break + if node in seen: + continue + seen.add(node) + stack.extend(n for n in edges.get(node, ()) if n in live) + return on_cycle diff --git a/core/tests/test_registry.py b/core/tests/test_registry.py index 3629bc17..8c78559d 100644 --- a/core/tests/test_registry.py +++ b/core/tests/test_registry.py @@ -158,3 +158,66 @@ def test_traits_are_names_the_model_offers_not_model_names(scanned): for spec in scanned.specs.values(): for trait in spec.requires: assert trait.islower() and " " not in trait + + +# --- ordering: what a bad relation must not take with it ------------------- + +def test_a_cycle_does_not_drop_the_modules_that_merely_depend_on_it(): + # A module saying "after B" is not itself circular. Once B is gone its + # relation has nothing to wait for -- exactly like a relation naming a + # module that was never installed. + from core.schema import validate + b = validate({"id": "b", "title": "B", "mount": "m", "after": ["c"]}) + c = validate({"id": "c", "title": "C", "mount": "m", "after": ["b"]}) + a = validate({"id": "a", "title": "A", "mount": "m", "after": ["b"]}) + d = validate({"id": "d", "title": "D", "mount": "m"}) + + ordered, rejected = order([a, b, c, d]) + assert {s.id for s in ordered} == {"a", "d"} + assert {s.id for s, _ in rejected} == {"b", "c"} + + +def test_a_longer_cycle_is_found_whole(): + from core.schema import validate + ring = [ + validate({"id": "x", "title": "X", "mount": "m", "after": ["z"]}), + validate({"id": "y", "title": "Y", "mount": "m", "after": ["x"]}), + validate({"id": "z", "title": "Z", "mount": "m", "after": ["y"]}), + ] + healthy = validate({"id": "ok", "title": "OK", "mount": "m", "after": ["x"]}) + ordered, rejected = order(ring + [healthy]) + assert [s.id for s in ordered] == ["ok"] + assert {s.id for s, _ in rejected} == {"x", "y", "z"} + + +def test_a_relation_may_not_overturn_the_stage_order(): + # Stage is the coarse order. Honouring "load after conditioning" would put a + # whole stage out of sequence to satisfy one module's relation. + from core.schema import validate + late = validate({"id": "late", "title": "L", "mount": "m", "stage": "post"}) + early = validate({"id": "early", "title": "E", "mount": "m", "stage": "load", + "after": ["mid"]}) + mid = validate({"id": "mid", "title": "M", "mount": "m", "stage": "conditioning"}) + + ordered, rejected = order([late, early, mid]) + ids = [s.id for s in ordered] + assert rejected == [], "the module is kept; only its impossible relation is dropped" + assert ids == ["early", "mid", "late"], f"stages out of order: {ids}" + + +def test_before_may_not_overturn_the_stage_order_either(): + from core.schema import validate + early = validate({"id": "early", "title": "E", "mount": "m", "stage": "load"}) + late = validate({"id": "late", "title": "L", "mount": "m", "stage": "post", + "before": ["early"]}) + ordered, _ = order([late, early]) + assert [s.id for s in ordered] == ["early", "late"] + + +def test_a_relation_within_a_stage_is_still_honoured(): + from core.schema import validate + first = validate({"id": "first", "title": "F", "mount": "m", "stage": "latent"}) + second = validate({"id": "second", "title": "S", "mount": "m", "stage": "latent", + "after": ["first"]}) + ordered, _ = order([second, first]) + assert [s.id for s in ordered] == ["first", "second"]