From eeda41a72bc22e6426c4e3718bb1dacc5a9ab597 Mon Sep 17 00:00:00 2001 From: Maarten Breddels Date: Mon, 28 Sep 2026 16:09:40 +0200 Subject: [PATCH] Fast renderer: do not lose handler and cleanup errors An audit of REACTON_FAST=1 against the default renderer (fuzzing both renderers, solara's test suite, and a production app) found fast-only bugs that the default renderer does not have: - An event handler exception raised while another thread renders was lost: force_update only set a flag that the running render cleared, and no path to the component was marked, so later renders skipped it. - An effect cleanup exception during unmount was lost, and closed widgets stayed on screen: marking the ancestors stopped at a stale flag inside the subtree being removed, so the live ancestors were never marked. Walking to the root costs only the tree depth. - The fix for a replaced parent widget did not cover a widget replaced by a shared component element, whose arguments are rendered in the same context: that still crashed with a KeyError. Both now share one helper. The default renderer is unchanged by this; the new tests run in both modes in CI and fail on the fast renderer without the fix. Co-Authored-By: Claude Opus 5.5 (1M context) --- reacton/core.py | 44 ++++++++----- reacton/core_test.py | 154 +++++++++++++++++++++++++++++++++++++++++++ reacton/ipyvue.py | 1 + 3 files changed, 183 insertions(+), 16 deletions(-) diff --git a/reacton/core.py b/reacton/core.py index 4211e13..08b1d01 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -210,6 +210,10 @@ def wrapper(*args, **kwargs): # we add it to exceptions_children, not exception_self # this allows a component to catch the exception of a direct child context.exceptions_children.append(e) + # force_update walks the whole tree, but when another thread is rendering it + # only sets a flag that this render may clear: mark the path to this context + # so the fast renderer does not skip it on the next render + _mark_needs_render_ancestors(context) rc.force_update() return wrapper @@ -234,8 +238,12 @@ def same_component(c1, c2): def _mark_needs_render_ancestors(context: "ComponentContext"): """Let the render phase find its way down to a context that needs work, without walking subtrees that do not.""" + # walk up to the root: stopping at the first ancestor that is already marked would + # assume its ancestors are marked too, which does not hold for a subtree that is + # being removed (it can carry a flag from before), so the live ancestors above it + # would stay unmarked and skip e.g. a cleanup exception parent = context.parent - while parent is not None and not parent.needs_render_descendant: + while parent is not None: parent.needs_render_descendant = True parent = parent.parent @@ -2489,8 +2497,8 @@ class _RenderContextFast(_RenderContext): # without walking either. # ------------------------------------------------------------------ - # > 0 while the render phase walks the new children of a widget that replaces a - # widget of another type (see _render) + # > 0 while the render phase walks the new children of a widget, or the arguments of a + # shared element, that replaces an element of another type (see _render_arguments) _replacing = 0 def _set_rerender_needed(self, reason: str): @@ -2541,18 +2549,7 @@ def _render(self, element: Element, default_key: str, parent_key: str): del context.children_next[key] # the element arguments are part of this component's element tree if el.kwargs: - el_reconciled = context.elements.get(key) - if el_reconciled is not None and el_reconciled.component != el.component: - # reconciliation replaces the widget at this key, and first removes the - # old subtree, including the component contexts in it: the walk below - # must not keep one of those as it is (see the fast path further down) - self._replacing += 1 - try: - self._visit_children(el, key, parent_key, self._render) - finally: - self._replacing -= 1 - else: - self._visit_children(el, key, parent_key, self._render) + self._render_arguments(el, key, parent_key) return assert isinstance(el.component, ComponentFunction) @@ -2560,7 +2557,7 @@ def _render(self, element: Element, default_key: str, parent_key: str): # arguments of a shared element belong to the context it is rendered in; # for non-shared component elements the component function decides # what ends up in the tree - self._visit_children(el, key, parent_key, self._render) + self._render_arguments(el, key, parent_key) context_previous = context.children_next.get(key) if context_previous is None: @@ -3065,6 +3062,21 @@ def _visit_children_values(self, value: Any, key: str, parent_key: str, f: Calla else: return value + def _render_arguments(self, el: Element, key: str, parent_key: str): + """Render the elements in the arguments of a widget element or a shared element.""" + assert self.context is not None + el_reconciled = self.context.elements.get(key) + replaced = el_reconciled is not None and el_reconciled.component != el.component + if replaced: + # a different component at this key: reconciliation removes the old element and + # everything below it, so nothing below the new one may be skipped as clean + self._replacing += 1 + try: + self._visit_children(el, key, parent_key, self._render) + finally: + if replaced: + self._replacing -= 1 + def _remove_stale_root_elements(self, parent_key): # remove stale elements of the root context itself # (child contexts are swept during their reconciliation) diff --git a/reacton/core_test.py b/reacton/core_test.py index 68b2bed..e988750 100644 --- a/reacton/core_test.py +++ b/reacton/core_test.py @@ -3819,3 +3819,157 @@ def Test(child): set_vertical(True) assert len(rc.find(widgets.Button)) == 1 rc.close() + + +def test_widget_replaced_by_shared_component_around_the_same_child(): + # like above, but the new element is a shared component element, whose arguments + # are rendered in this context too + set_flip = lambda x: None # noqa + + @react.component + def Child(): + return w.Label(value="child") + + @react.component + def Row(children): + return w.HBox(children=children) + + @react.component + def App(): + nonlocal set_flip + flip, set_flip = react.use_state(False) + child = react.use_memo(lambda: Child().key("child"), []) + if flip: + return w.VBox(children=[Row(children=[child]).shared()]) + return w.VBox(children=[w.VBox(children=[child])]) + + box, rc = react.render(App(), handle_error=False) + set_flip(True) + hbox = box.children[0].children[0] + assert isinstance(hbox, widgets.HBox) + assert hbox.children[0].comm is not None + assert hbox.children[0].value == "child" + rc.close() + + +def test_effect_cleanup_exception_on_unmount_reaches_exception_handler(): + # a render exception marks the subtree dirty, the handler replaces the subtree, and a + # cleanup in the removed subtree raises: the handler must see that exception too, and + # no closed widget may stay on screen + cleanups = [] + set_crash = lambda x: None # noqa + + @react.component + def Crasher(): + nonlocal set_crash + crash, set_crash = react.use_state(False) + if crash: + raise ValueError("render boom") + return w.Label(value="ok") + + @react.component + def Unsubscriber(): + def effect(): + def cleanup(): + cleanups.append("cleanup") + raise IndexError("cleanup boom") + + return cleanup + + use_effect(effect, []) + return w.Label(value="subscribed") + + @react.component + def Middle(): + return w.VBox(children=[Crasher(), Unsubscriber()]) + + @react.component + def Handler(): + exception, clear = react.use_exception() + if exception is not None: + return w.VBox(children=[w.Label(value=f"caught {exception!r}")]) + return w.VBox(children=[Middle()]) + + @react.component + def App(): + return w.VBox(children=[Handler()]) + + box, rc = react.render(App(), handle_error=False) + set_crash(True) + assert cleanups == ["cleanup"] + shown = box.children[0].children[0].children[0] + assert shown.comm is not None + assert "cleanup boom" in shown.value + rc.close() + + +def test_event_handler_exception_while_other_thread_renders(): + # the handler exception is routed via force_update, which during a render (on another + # thread) only sets a flag; the exception must still reach the handler on a later render + started = threading.Event() + go = threading.Event() + set_slow = lambda x: None # noqa + set_other = lambda x: None # noqa + + @react.component + def Clicky(): + def on_click(): + raise ValueError("click boom") + + return w.Button(description="click", on_click=on_click) + + @react.component + def Middle(): + return w.VBox(children=[Clicky()]) + + @react.component + def Slow(n): + if n == 1: + started.set() + # the click must happen while this render is in progress + assert go.wait(5) + return w.Label(value=f"slow {n}") + + @react.component + def Other(): + nonlocal set_other + value, set_other = react.use_state(0) + return w.Label(value=f"other {value}") + + @react.component + def App(): + nonlocal set_slow + exception, clear = react.use_exception() + n, set_slow = react.use_state(0) + # the same element every render: an unchanged subtree + middle = react.use_memo(lambda: Middle(), []) + if exception is not None: + return w.HTML(value=f"caught {exception!r}") + return w.VBox(children=[middle, Slow(n=n), Other()]) + + box, rc = react.render(App(), handle_error=False) + button = rc.find(widgets.Button).widget + + clicked_during_render = [] + + def click_during_render(): + try: + if started.wait(5): + button.click() + clicked_during_render.append(True) + finally: + go.set() + + thread = threading.Thread(target=click_during_render) + thread.start() + try: + set_slow(1) + finally: + thread.join(5) + assert not thread.is_alive() + assert clicked_during_render == [True] + # the exception reaches the handler on the next render (in both renderers) + set_other(1) + assert isinstance(box.children[0], widgets.HTML) + assert "click boom" in box.children[0].value + rc.close() diff --git a/reacton/ipyvue.py b/reacton/ipyvue.py index d805834..f0976e2 100644 --- a/reacton/ipyvue.py +++ b/reacton/ipyvue.py @@ -41,6 +41,7 @@ def handler(*args): # we add it to exceptions_children, not exception_self # this allows a component to catch the exception of a direct child context.exceptions_children.append(e) + react.core._mark_needs_render_ancestors(context) rc.force_update() vue_widget.on_event(event_and_modifiers, handler)