Fast renderer: do not lose handler and cleanup errors

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,26 +2549,15 @@ 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)
         if el.is_shared and (el.args or el.kwargs):
             # 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)

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()

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)