Do not hold a global lock while a widget is constructed

reacton/core.py

@@ -88,30 +88,43 @@ def get():
 # via the widget constructed hook: diffing the global widgets dict per widget
 # creation is O(total widgets), and solara replaces that dict with a context
 # aware mapping we should not depend on.
-_construction_recording: Optional[List["widgets.Widget"]] = None
+# The recording is per thread, so no lock is held while a widget is constructed:
+# construction opens a comm, and in a server that send can wait for a slow client,
+# which with a process wide lock stalled widget creation on every other thread.
+class _ConstructionRecording(threading.local):
+    # where the widgets constructed on this thread go, between _start_recording_constructed and
+    # _stop_recording_constructed
+    constructed: Optional[List["widgets.Widget"]] = None
+
+
+_construction_recording = _ConstructionRecording()
 _chained_construction_callback: Optional[Callable] = None
 
 
 def _record_constructed_widget(widget: "widgets.Widget"):
-    if _construction_recording is not None:
-        _construction_recording.append(widget)
+    constructed = _construction_recording.constructed
+    if constructed is not None:
+        constructed.append(widget)
     if _chained_construction_callback is not None:
         _chained_construction_callback(widget)
 
 
-def _start_recording_constructed(recording: List["widgets.Widget"]):
-    global _construction_recording, _chained_construction_callback
+def _start_recording_constructed(recording: List["widgets.Widget"]) -> Optional[List["widgets.Widget"]]:
+    """Record the widgets this thread constructs; returns what to pass to _stop_recording_constructed."""
+    global _chained_construction_callback
     current = getattr(widgets.Widget, "_widget_construction_callback", None)
     if current is not _record_constructed_widget:
         # first time, or someone else registered a callback after us: chain it
         _chained_construction_callback = current
         widgets.Widget.on_widget_constructed(_record_constructed_widget)
-    _construction_recording = recording
+    # a widget whose constructor renders records in its own list; the outer recording continues afterwards
+    previous = _construction_recording.constructed
+    _construction_recording.constructed = recording
+    return previous
 
 
-def _stop_recording_constructed():
-    global _construction_recording
-    _construction_recording = None
+def _stop_recording_constructed(previous: Optional[List["widgets.Widget"]]):
+    _construction_recording.constructed = previous
 
 
 _last_rc = None  # used for testing
@@ -334,7 +347,6 @@ class Element(Generic[W]):
     # to make every unique on_value callback to a unique wrapper
     # so that we can remove the listeners
     _callback_wrappers: Dict[Tuple[str, str, Callable], Callable] = {}
-    create_lock: ContextManager = threading.Lock()
     _shared = False
 
     def __init__(self, component, args=None, kwargs=None):
@@ -499,32 +511,30 @@ def _create_widget(self, kwargs):
         # we can't use our own kwarg, since that contains elements, not widgets
         kwargs, listeners = self._split_kwargs(kwargs)
         assert isinstance(self.component, ComponentWidget)
-        # The recording is global state, so we need a lock.
-        with self.create_lock:
-            rc = get_render_context(required=True)
-            recorded: List[widgets.Widget] = []
-            _start_recording_constructed(recorded)
+        rc = get_render_context(required=True)
+        recorded: List[widgets.Widget] = []
+        previous_recording = _start_recording_constructed(recorded)
+        try:
             try:
-                try:
-                    widget = self.component.widget(**kwargs)
-                    hold_trait_notifications = widget.hold_trait_notifications
-
-                    @contextlib.contextmanager
-                    def hold_trait_notifications_extra(*args, **kwargs):
-                        with rc, hold_trait_notifications(*args, **kwargs):
-                            yield
-
-                    widget.hold_trait_notifications = hold_trait_notifications_extra
-
-                    if self._meta:
-                        widget._react_meta = dict(self._meta)
-                except Exception as e:
-                    raise RuntimeError(f"Could not create widget {self.component.widget} with {kwargs}") from e
-                for name, callback in listeners.items():
-                    if callback is not None:
-                        self._add_widget_event_listener(widget, name, callback)
-            finally:
-                _stop_recording_constructed()
+                widget = self.component.widget(**kwargs)
+                hold_trait_notifications = widget.hold_trait_notifications
+
+                @contextlib.contextmanager
+                def hold_trait_notifications_extra(*args, **kwargs):
+                    with rc, hold_trait_notifications(*args, **kwargs):
+                        yield
+
+                widget.hold_trait_notifications = hold_trait_notifications_extra
+
+                if self._meta:
+                    widget._react_meta = dict(self._meta)
+            except Exception as e:
+                raise RuntimeError(f"Could not create widget {self.component.widget} with {kwargs}") from e
+            for name, callback in listeners.items():
+                if callback is not None:
+                    self._add_widget_event_listener(widget, name, callback)
+        finally:
+            _stop_recording_constructed(previous_recording)
         widgets_dict = _get_widgets_dict()
         orphans = {w.model_id for w in recorded if w is not widget and w.comm is not None and w.model_id in widgets_dict}
         return widget, orphans

reacton/core_test.py

@@ -4304,3 +4304,108 @@ def Test():
     assert log == ["run"]
     rc.close()
     assert log == ["run", "cleanup"]
+
+
+def test_widget_construction_does_not_block_other_threads():
+    # A widget construction that blocks (in a server: the comm_open send to a client that stopped
+    # reading) used to hold a process wide lock, so no other thread could create widgets.
+    entered, release = threading.Event(), threading.Event()
+
+    class BlockingButton(ipywidgets.Button):
+        def __init__(self, **kwargs):
+            super().__init__(**kwargs)
+            if threading.current_thread().name == "slow":
+                entered.set()
+                release.wait(10)
+
+    BlockingButtonElement = reacton.core.ComponentWidget(BlockingButton)
+
+    @reacton.component
+    def Slow():
+        return BlockingButtonElement(description="slow")
+
+    @reacton.component
+    def Fast():
+        return w.Button(description="fast")
+
+    results: dict = {}
+
+    def render(name, component):
+        results[name] = reacton.render(component(), handle_error=False)
+
+    slow = threading.Thread(target=render, args=("slow", Slow), name="slow", daemon=True)
+    slow.start()
+    try:
+        assert entered.wait(10)
+        fast = threading.Thread(target=render, args=("fast", Fast), name="fast", daemon=True)
+        fast.start()
+        fast.join(5)
+        assert "fast" in results, "widget creation waited for a widget construction on another thread"
+    finally:
+        release.set()
+        slow.join(10)
+    for name in ["slow", "fast"]:
+        box, rc = results[name]
+        button = box.children[0]
+        # side-effect widgets are still attributed to their own render context
+        assert rc._orphans[button.model_id] == {button.layout.model_id, button.style.model_id}
+        rc.close()
+
+
+def test_widget_created_by_another_thread_is_not_an_orphan():
+    # the recording is per thread: a widget that user code creates on another thread while a
+    # render constructs a widget must not be recorded as that widget's side effect (and closed
+    # with it)
+    entered, release = threading.Event(), threading.Event()
+
+    class BlockingButton(ipywidgets.Button):
+        def __init__(self, **kwargs):
+            super().__init__(**kwargs)
+            entered.set()
+            release.wait(10)
+
+    BlockingButtonElement = reacton.core.ComponentWidget(BlockingButton)
+
+    @reacton.component
+    def Slow():
+        return BlockingButtonElement(description="slow")
+
+    results: dict = {}
+    render = threading.Thread(target=lambda: results.update(result=reacton.render(Slow(), handle_error=False)), daemon=True)
+    render.start()
+    try:
+        assert entered.wait(10)
+        unrelated = ipywidgets.IntSlider()
+    finally:
+        release.set()
+        render.join(10)
+    box, rc = results["result"]
+    button = box.children[0]
+    assert unrelated.model_id not in rc._orphans[button.model_id]
+    rc.close()
+    assert unrelated.comm is not None, "a widget of another thread was closed as an orphan"
+    for widget in [unrelated, unrelated.layout, unrelated.style]:
+        widget.close()
+
+
+def test_record_constructed_nested():
+    # a widget whose constructor renders reacton records in its own list, and the outer recording
+    # continues afterwards
+    outer: list = []
+    inner: list = []
+    previous_outer = core._start_recording_constructed(outer)
+    try:
+        before = ipywidgets.Button()
+        previous_inner = core._start_recording_constructed(inner)
+        try:
+            nested = ipywidgets.Button()
+        finally:
+            core._stop_recording_constructed(previous_inner)
+        after = ipywidgets.Button()
+    finally:
+        core._stop_recording_constructed(previous_outer)
+    assert core._construction_recording.constructed is None
+    assert before in outer and after in outer and nested not in outer
+    assert nested in inner and before not in inner
+    for widget in outer + inner:
+        widget.close()