From aa2f29fb3a678e7143fad50704a81e3b7734d2ca Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Tue, 29 Sep 2026 10:23:24 +0200 Subject: [PATCH 1/2] Fast renderer: clean up removed components before new effects run When a component replaces another one in a container, the default renderer runs the effect cleanups of the removed component before the effects of the new one. The fast renderer removed stale elements only after the whole context was reconciled, so the new effect ran first. An effect that sets a global value and resets it in its cleanup then ended with the reset value, and with two failing cleanups a different exception reached use_exception. The fast renderer now runs the effect cleanups of stale components at the same point as the default renderer. Their widgets are still closed later, after the parent got its new children, as before. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 61 +++++++++++++++++++++++++++ reacton/core_test.py | 99 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/reacton/core.py b/reacton/core.py index 4937bea..e474053 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1205,6 +1205,11 @@ class ComponentContext: # the render phase skipped this whole subtree (nothing changed), so the # reconciliation phase can reuse the previous result without walking clean_subtree: bool = False + # Fast renderer only: stale component effect cleanups are run at the same + # point where the default renderer removes stale elements, while widget + # closing stays deferred to the fast renderer's stale sweep. + fast_stale_effect_keys: Optional[List[str]] = None + fast_stale_effects_cleaned: Set[str] = field(default_factory=set) # elements created in this context go there owns: Set[Element] = field(default_factory=set) @@ -2523,6 +2528,8 @@ def _render(self, element: Element, default_key: str, parent_key: str): # the root element of a component determines which keys are in use, # everything else is stale and gets removed during reconciliation context.used_keys.clear() + context.fast_stale_effect_keys = None + context.fast_stale_effects_cleaned.clear() el = element key = el._key @@ -2923,6 +2930,7 @@ def _reconsolidate(self, el: Element, default_key: str, parent_key: str): self._shared_elements.add(el) assert el in self._shared_elements_next self._shared_elements_next.remove(el) + self._cleanup_stale_effects_for_context(context, parent_key) def _process_effects(self, child_context: "ComponentContext", context: "ComponentContext"): # NOTE: effect/cleanup exceptions are recorded on the context of the @@ -2965,6 +2973,59 @@ def _store_widget(self, context: "ComponentContext", el: Element, key: str, widg else: context.widgets[key] = widget + def _cleanup_stale_effects_for_context(self, context: "ComponentContext", parent_key: str): + if context.fast_stale_effect_keys is None: + context.fast_stale_effect_keys = sorted(set(context.elements) - context.used_keys) + while context.fast_stale_effect_keys: + stale_key = context.fast_stale_effect_keys.pop(0) + if stale_key not in context.elements or stale_key in context.fast_stale_effects_cleaned: + continue + self.context = context + self._cleanup_stale_effects(context.elements[stale_key], stale_key, parent_key) + + def _cleanup_stale_effects(self, el: Element, default_key: str, parent_key: str): + key = el._key + if key is None: + key = default_key + assert key is not None + context = self.context + assert context is not None + if key in context.fast_stale_effects_cleaned: + return + if el.is_shared and (el in self._shared_elements_next or el not in self._shared_elements): + return + context.fast_stale_effects_cleaned.add(key) + + if isinstance(el.component, ComponentFunction): + if el.is_shared: + self._visit_children(el, key, parent_key, self._cleanup_stale_effects) + child_context = context.children.get(key) + if child_context is None: + return + try: + self.context = child_context + child_context.exceptions_self = [] + child_context.exceptions_children = [] + for effect in child_context.effects: + try: + if not effect._cleaned_up: + effect.cleanup() + except BaseException as e: + effect._cleaned_up = True + logger.exception("Effect cleanup %r raised exception %r", effect.callable, e) + child_context.exceptions_self.append(e) + self._set_rerender_needed("Exception ocurred during effect") + _mark_needs_render_ancestors(child_context) + assert child_context.root_element is not None + self._cleanup_stale_effects(child_context.root_element, "/", parent_key=join_key(parent_key, key)) + finally: + self.context = context + if child_context.exceptions_self or child_context.exceptions_children and not child_context.exception_handler: + context.exceptions_children.extend(child_context.exceptions_self) + context.exceptions_children.extend(child_context.exceptions_children) + else: + self._visit_children(el, key, parent_key, self._cleanup_stale_effects) + def _remove_element(self, el: Element, default_key: str, parent_key): key = el._key if key is None: diff --git a/reacton/core_test.py b/reacton/core_test.py index fb4b9cb..581253c 100644 --- a/reacton/core_test.py +++ b/reacton/core_test.py @@ -4088,3 +4088,102 @@ def cleanup(): assert box.children[0].value == "value 1" finally: rc.close() + + +def test_stale_component_cleanup_runs_before_new_sibling_effect(): + active = None + set_show_old = lambda x: None # noqa + + def owner(name): + def effect(): + nonlocal active + active = name + + def cleanup(): + nonlocal active + active = None + + return cleanup + + return effect + + @react.component + def Old(): + react.use_effect(owner("old"), []) + return w.Label(value="old") + + @react.component + def New(): + react.use_effect(owner("new"), []) + return w.Label(value="new") + + @react.component + def App(): + nonlocal set_show_old + show_old, set_show_old = react.use_state(True) + children = [w.Label(value="first").key("first")] + if show_old: + children.append(Old().key("old")) + else: + children.append(New().key("new")) + return w.VBox(children=children) + + box, rc = react.render(App(), handle_error=False) + assert active == "old" + set_show_old(False) + assert active == "new" + rc.close() + + +def test_stale_component_cleanup_order_with_nested_removed_components(): + log = [] + set_show_old = lambda x: None # noqa + + def logger(name): + def effect(): + log.append(f"{name} run") + + def cleanup(): + log.append(f"{name} cleanup") + + return cleanup + + return effect + + @react.component + def Inner(name): + react.use_effect(logger(f"{name}-inner"), []) + return w.Label(value=f"{name}-inner") + + @react.component + def Old(name): + react.use_effect(logger(f"{name}-outer"), []) + return w.VBox(children=[Inner(name=name)]) + + @react.component + def New(): + react.use_effect(logger("new"), []) + return w.Label(value="new") + + @react.component + def App(): + nonlocal set_show_old + show_old, set_show_old = react.use_state(True) + children = [w.Label(value="first").key("first")] + if show_old: + children.extend([Old(name="z").key("z"), Old(name="a").key("a")]) + else: + children.append(New().key("new")) + return w.VBox(children=children) + + box, rc = react.render(App(), handle_error=False) + log.clear() + set_show_old(False) + assert log == [ + "a-outer cleanup", + "a-inner cleanup", + "z-outer cleanup", + "z-inner cleanup", + "new run", + ] + rc.close() From f474df4a2d98c4fe3686f4d1cdbf3e887250c878 Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Tue, 29 Sep 2026 10:45:09 +0200 Subject: [PATCH 2/2] Linear stale cleanup, free its bookkeeping on close, fuzz cleanups Review found that taking stale keys from the front of a list is quadratic in the number of stale keys, that a closed context kept the removed keys alive through its new bookkeeping fields, and that the cleanup loop left self.context changed. The fuzz test only logged effect runs, so it could not see a difference in cleanup order: with cleanups logged, 7 of its 12 cases fail on master and all pass with this change. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 21 ++++++++++++++------- reacton/fuzz_test.py | 6 ++++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/reacton/core.py b/reacton/core.py index e474053..40c0eeb 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1268,6 +1268,8 @@ def _teardown_component_context(context: ComponentContext): context.exceptions_self = [] context.exceptions_children = [] context.context_managers = [] + context.fast_stale_effect_keys = None + context.fast_stale_effects_cleaned = set() @dataclass @@ -2975,13 +2977,18 @@ def _store_widget(self, context: "ComponentContext", el: Element, key: str, widg def _cleanup_stale_effects_for_context(self, context: "ComponentContext", parent_key: str): if context.fast_stale_effect_keys is None: - context.fast_stale_effect_keys = sorted(set(context.elements) - context.used_keys) - while context.fast_stale_effect_keys: - stale_key = context.fast_stale_effect_keys.pop(0) - if stale_key not in context.elements or stale_key in context.fast_stale_effects_cleaned: - continue - self.context = context - self._cleanup_stale_effects(context.elements[stale_key], stale_key, parent_key) + # reversed, so we can pop from the end and still go in sorted order + context.fast_stale_effect_keys = sorted(set(context.elements) - context.used_keys, reverse=True) + context_prev = self.context + try: + while context.fast_stale_effect_keys: + stale_key = context.fast_stale_effect_keys.pop() + if stale_key not in context.elements or stale_key in context.fast_stale_effects_cleaned: + continue + self.context = context + self._cleanup_stale_effects(context.elements[stale_key], stale_key, parent_key) + finally: + self.context = context_prev def _cleanup_stale_effects(self, el: Element, default_key: str, parent_key: str): key = el._key diff --git a/reacton/fuzz_test.py b/reacton/fuzz_test.py index a10f8d5..e8d1feb 100644 --- a/reacton/fuzz_test.py +++ b/reacton/fuzz_test.py @@ -81,6 +81,12 @@ def effect(): if state % 5 == 4: set_state(state + 1) + def cleanup(): + # both renderers must also run cleanups in the same order + log.append(("cleanup", id, state)) + + return cleanup + react.use_effect(effect, [state]) children: List[Any] = [w.Label(value=f"node {id} {state}")] for i in range(rnd.randint(0, 4)):