Skip to content

Clear EventLoopThread.loop before closing it - #739

Merged
puddly merged 1 commit into
devfrom
zigpy-bot/thread-clear-loop-before-close
Jul 29, 2026
Merged

Clear EventLoopThread.loop before closing it#739
puddly merged 1 commit into
devfrom
zigpy-bot/thread-clear-loop-before-close

Conversation

@zigpy-review-bot

Copy link
Copy Markdown
Collaborator

Follow-up to the review thread on #727 (#727 (comment)).

#727 fixed the RuntimeError: Event loop is closed flake on the read side, in force_stop. The write side that creates the window is still in _thread_main:

finally:
    self.loop.close()
    self.loop = None

Between those two statements self.loop names an already-closed loop. This swaps them, so the loop reference is published as None before the loop is closed:

finally:
    loop, self.loop = self.loop, None
    loop.close()

Two things follow:

  • self.loop is either None or an open loop, for every reader — not just the one force_stop guards. run_coroutine_threadsafe() (bellows/thread.py:17-20) passes self.loop straight to asyncio.run_coroutine_threadsafe, which raises the same RuntimeError on a closed loop, and start()s restart guard reads it too. A None loop is already handled correctly by start(), which falls through to creating a new one.
  • The exiting thread stops writing self.loop after it clears it. Previously the trailing self.loop = None ran after close(), so a start() racing the shutdown could have its freshly installed loop clobbered back to None by the thread that was on its way out. With the swap there is no write after the handoff.

#727s contextlib.suppress(RuntimeError) in force_stop is still needed and stays — a caller can snapshot the loop just before the worker reaches close(), and no ordering here can prevent that.

Test

test_thread_clears_loop_before_closing wraps the worker loops close() and records what self.loop is at the moment close is called, asserting it is already None. It fails on dev (assert <_UnixSelectorEventLoop ...> is None) and passes with the change, so it pins the ordering rather than just the end state.

Verification
  • pytest tests/441 passed (Python 3.13.11)
  • pytest tests/test_thread.py::test_thread_clears_loop_before_closing with bellows/thread.py reverted to dev1 failed, confirming the test is not vacuous
  • pre-commit: autoflake, black, flake8, isort, codespell, ruff pass. pyupgrade and mypy fail identically on untouched files (ast.Str removed in 3.14 / missing typed-ast), so those are local hook-env breakage, not this change — CI is the real check.

`_thread_main`'s `finally` closed the loop and only then set `self.loop = None`,
so between those two statements `self.loop` named an already-closed loop — the
window #727 had to defend against with an `is_closed()` guard plus a suppressed
`RuntimeError` in `force_stop`.

Publishing `None` first makes the invariant "`self.loop` is either `None` or an
open loop" hold for every reader, and it also means the exiting thread never
writes `self.loop` again after that point — previously the trailing
`self.loop = None` could clobber the loop a concurrent `start()` had just
installed.

#727's `suppress(RuntimeError)` in `force_stop` stays: a caller can still
snapshot the loop just before `close()` runs.
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.55%. Comparing base (b515556) to head (2fa0433).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #739   +/-   ##
=======================================
  Coverage   99.55%   99.55%           
=======================================
  Files          64       64           
  Lines        4263     4263           
=======================================
  Hits         4244     4244           
  Misses         19       19           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants