diff --git a/reacton/core.py b/reacton/core.py index 87b9687..3a37526 100644 --- a/reacton/core.py +++ b/reacton/core.py @@ -1344,6 +1344,9 @@ def _keep_keyed_child(self, context: ComponentContext, el: Element, key: str) -> def __init__(self, element: Element, container: widgets.Widget = None, children_trait="children", handle_error: bool = True, initial_state=None): self.element = element + # the last requested root element: the thread that holds the render lock copies it to + # self.element at the start of each pass (see render) + self._element_next = element self.container = container self.children_next_trait = children_trait self.first_render = True @@ -1454,6 +1457,7 @@ def collect(context: ComponentContext): # widgets reference their elements, whose kwargs hold user callbacks - which # capture use_state setters and therefore this render context self.element = None # type: ignore + self._element_next = None # type: ignore self.container = None self.last_root_widget = None self._old_element_ids.clear() @@ -1607,7 +1611,7 @@ def force_update(self): # a forced update re-walks the whole tree, no subtree skipping self._walk_all = True if not self._is_rendering: - self.render(self.element, self.container) + self.render() def use_effect(self, effect: EffectCallable, dependencies=None): assert self.context is not None @@ -1631,15 +1635,15 @@ def use_effect(self, effect: EffectCallable, dependencies=None): def update(self, element: Element): self._walk_all = True if self._is_rendering: - self.element = element + self._element_next = element self._rerender_needed_reasons.append(RerenderReason(reason="root element changed")) self._rerender_needed = True else: - self.render(element, self.container) + self.render(element) def _possible_rerender(self): if not self._is_rendering and self._batch_counter.current() == 0: - self.render(self.element, self.container) + self.render() else: logger.info("No render phase triggered, already rendering") @@ -1672,25 +1676,31 @@ def _discard_aborted_pass(self): self._shared_elements_next = set() self.context = self.context_root - def render(self, element: Element, container: widgets.Widget = None): + def render(self, element: Optional[Element] = None, container: widgets.Widget = None): # render + consolidate + # We never wait for the render lock: the caller may hold a lock that the render needs (a + # deadlock). So we first mark the request, then only try the lock. When another thread holds + # it, that thread renders the request: it takes _element_next at the start of each pass, and + # looks at _rerender_needed again after it released the lock (at the end of this method). + if self._lock_thread == threading.current_thread(): + raise RuntimeError("Recursive render detected (avoided deadlock), current thread: %r" % threading.current_thread()) widget = None if container is None: container = self.container - was_locked = False - if self.thread_lock.locked(): - if self._lock_thread == threading.current_thread(): - raise RuntimeError("Recursive render detected (avoided deadlock), current thread: %r" % threading.current_thread()) - logger.info( - "Render phase still in progress, waiting for mutex to release (locked obtained by %r, we are in thread %r)", - self._lock_thread, - threading.current_thread(), - ) - was_locked = True - with self.thread_lock: + if element is not None: + self._element_next = element + if self._rerender_needed is False: + self._rerender_needed_reasons.append(RerenderReason(reason="render requested")) + self._rerender_needed = True + locked = False + try: + # acquire inside the try, in one statement with the assignment: an exception raised + # on the next line (an interrupt, a cancel from a trace function) still releases the lock + locked = self.thread_lock.acquire(blocking=False) + if not locked: + logger.info("Render phase in progress in thread %r, leaving the render to it", self._lock_thread) + return container self._lock_thread = threading.current_thread() - if was_locked: - logger.info("Mutex released, continuing render phase") if self._closing or self.context is None: # close() won the race for the lock (a disconnect can close the # kernel while an update was waiting to render): the tree is @@ -1702,11 +1712,11 @@ def render(self, element: Element, container: widgets.Widget = None): in_render_phase = True try: local.rc = self - self.element = element - del element main_render_phase = not self._is_rendering render_count = self.render_count # make a copy + # clear before taking the element: a request that comes in between is seen by the loop self._rerender_needed = False + self.element = self._element_next logger.info("Render phase: %r %r of %r", self.render_count, "main" if main_render_phase else "(nested)", self.element) self.render_count += 1 self._is_rendering = True @@ -1757,6 +1767,7 @@ def format(reason: RerenderReason): raise RuntimeError(msg) logger.info("Entering nested render phase: %r", self._rerender_needed_reasons[-1]) self._rerender_needed = False + self.element = self._element_next self._shared_elements_next = set() self.context.exception_handler = False self.context.exceptions_children = [] @@ -1825,13 +1836,7 @@ def format(reason: RerenderReason): # an exception bubbled up during reconsolidate break - # A setter on another thread sets _rerender_needed and then reads _is_rendering: - # while that is True, it leaves the render to us. So clear _is_rendering before - # the last look at _rerender_needed: either we see its change here, or the setter - # sees that we are done and renders the change itself. - self._is_rendering = False if self._rerender_needed: - self._is_rendering = True logger.info("Need rerender after reconsolidation: %r", self._rerender_needed_reasons[-1]) stable = False else: @@ -1869,6 +1874,9 @@ def format(reason: RerenderReason): # while a *different* thread holds the lock (false "Recursive render") self._lock_thread = None assert self.context is self.context_root + finally: + if locked: + self.thread_lock.release() exceptions = [*self.context.exceptions_children, *self.context_root.exceptions_self] if exceptions: @@ -1890,6 +1898,11 @@ def format(reason: RerenderReason): return self.render(w.HTML(value="
" + value + "", layout=w.Layout(overflow="auto")), self.container) else: raise exc + # A request from another thread (a setter, update(), render()) sets _rerender_needed, and leaves + # the render to us while we render or hold the render lock. We released the lock above, and look + # again now: either we see the request here, or that thread got the lock and renders it itself. + if self._rerender_needed: + self._possible_rerender() return widget def _render(self, element: Element, default_key: str, parent_key: str): diff --git a/reacton/threads_test.py b/reacton/threads_test.py index b1a8c98..ebfeaf3 100644 --- a/reacton/threads_test.py +++ b/reacton/threads_test.py @@ -1,5 +1,8 @@ +import logging import threading +import pytest + import reacton import reacton.ipywidgets as w @@ -69,3 +72,68 @@ def report_progress(): assert not other.is_alive() assert box.children[0].description == f"1 {n}" rc.close() + + +@pytest.mark.parametrize("request_render", ["set_state", "update", "render", "force_update"]) +def test_render_request_while_holding_a_user_lock_does_not_deadlock(request_render): + # Another thread holds its own lock while it asks for a render (a state change, update(), render() + # or force_update()), and the render on this thread takes that lock in an effect. If the other + # thread waits for the render lock (held by this thread), this thread waits for the user lock + # (held by the other thread): a deadlock. The effect uses a timeout to break it. + user_lock = threading.RLock() + deadlocked = [] + setters = {} + + @reacton.component + def Test(label=""): + a, setters["a"] = reacton.use_state(0) + b, setters["b"] = reacton.use_state(0) + + def effect(): + if user_lock.acquire(timeout=2): + user_lock.release() + else: + deadlocked.append(True) + + reacton.use_effect(effect, [a]) + return w.Button(description=f"{label}{a} {b}") + + box, rc = reacton.render(Test(), handle_error=False) + requests = { + "set_state": lambda: setters["b"](1), + "update": lambda: rc.update(Test(label="new ")), + "render": lambda: rc.render(Test(label="new ")), + "force_update": lambda: rc.force_update(), + } + expected = {"set_state": "1 1", "update": "new 1 0", "render": "new 1 0", "force_update": "1 0"} + + def request_holding_user_lock(): + with user_lock: + requests[request_render]() + + other = threading.Thread(target=request_holding_user_lock) + + class RequestAtRenderStart(logging.Filter): + # "Render phase: " is logged after the render took the render lock, but before it renders + def filter(self, record): + if str(record.msg).startswith("Render phase: ") and other.ident is None: + other.start() + # returns at once when the other thread does not wait for the render lock + other.join(0.5) + return True + + logger = logging.getLogger("reacton") + level = logger.level + request = RequestAtRenderStart() + logger.setLevel(logging.INFO) + logger.addFilter(request) + try: + setters["a"](1) + finally: + logger.removeFilter(request) + logger.setLevel(level) + other.join(TIMEOUT) + assert not other.is_alive() + assert not deadlocked, "the other thread waited for the render lock while it held the user lock" + assert box.children[0].description == expected[request_render] + rc.close()