Fix "Event loop is closed" race in EventLoopThread.force_stop#727
Conversation
The worker thread can close the loop between the None check and call_soon_threadsafe. Snapshot the loop, also guard on is_closed(), and swallow the RuntimeError if it is closed after the check.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #727 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4181 4184 +3
=======================================
+ Hits 4162 4165 +3
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The pre-commit ruff hook rejects the try/except-pass form (SIM105).
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Verified the root-cause analysis and the fix — this is correct and I'd merge it.
Root cause confirmed. _thread_main's finally does self.loop.close() and self.loop = None as two separate statements, so a concurrent force_stop() can observe a non-None but already-closed loop. I reproduced the resulting failure directly:
>>> loop = asyncio.new_event_loop(); loop.close()
>>> loop.call_soon_threadsafe(lambda: None)
RuntimeError: Event loop is closed
That matches the reported teardown flake exactly: test_thread_already_stopped calls force_stop() twice, then the fixture teardown calls it a third time — right in the window where the worker thread is tearing the loop down.
The fix is sound on all three counts:
- Snapshotting
self.loopinto a local also fixes a second latent bug that isn't in the PR description: the oldcancel_tasks_and_stop_loopclosure dereferencedself.loopwhen it ran on the worker thread, where it could already have been reassigned toNone→AttributeError. Capturing the local closes that too. - The
is_closed()guard matches the idiom already used instart()(self.loop is not None and not self.loop.is_closed()), so the two lifecycle entry points are now consistent. - The
contextlib.suppress(RuntimeError)is scoped to the singlecall_soon_threadsafe()call, which is the only statement that can raise for this reason. Since the race is inherently unwinnable without a lock (call_soon_threadsafeis not atomic with respect toclose()), catching it is the right call rather than a workaround. Usingsuppressinstead oftry/exceptalso means no# pragma: no coveris needed — thewithline itself executes on the covered path, which is why Codecov reports 100% patch coverage.
No behavioural change for the non-racing paths: a closed or absent loop now no-ops, which is the intended idempotent-stop semantics that uart.connect() relies on (it calls force_stop() from both the exception path and a add_done_callback, so a double-stop is normal there).
Checks: full suite green locally on 3.13 (424 passed), tests/test_thread.py green over 5 consecutive runs, and all CI legs (3.11–3.14) pass. A second-opinion static review flagged nothing.
Two optional notes below — neither blocks merge.
Nit — the PR description is stale. It says "the race-only except is marked # pragma: no cover", but the follow-up commit replaced the try/except with contextlib.suppress and there's no pragma in the final diff. Worth a quick edit so the description doesn't mislead if it ends up in the squash commit message.
|
|
||
| self.loop.call_soon_threadsafe(cancel_tasks_and_stop_loop) | ||
| # The worker thread may close the loop after our is_closed() check. | ||
| with contextlib.suppress(RuntimeError): |
There was a problem hiding this comment.
Optional follow-up, not for this PR: the read side is now safe, but the write side that creates the window is still bellows/thread.py:29-31:
finally:
self.loop.close()
self.loop = NoneSwapping those to publish None before closing narrows the window in which self.loop names a closed loop:
finally:
loop, self.loop = self.loop, None
loop.close()That doesn't remove the need for this suppress — a caller can still snapshot the loop just before close() runs — but it also protects the two other readers of self.loop that this PR doesn't touch: run_coroutine_threadsafe() (line 19) passes self.loop straight to asyncio.run_coroutine_threadsafe, which raises the same RuntimeError on a closed loop, and start()'s restart guard (line 35). It's safe for start() either way, since a None loop already falls through to creating a new one.
Entirely fine to leave for a separate change — the flake this PR targets is fixed as-is.
Independent of any feature work and pre-exists on
dev; surfaced while stabilizing CI.tests/test_thread.py::test_thread_already_stoppedintermittently errors at teardown withRuntimeError: Event loop is closed(observed on the 3.11/3.13 CI legs; passes on 3.12/3.14 and locally).Root cause: the worker thread's
_thread_mainfinallyrunsself.loop.close()and thenself.loop = Noneas two separate statements.
force_stop'sif self.loop is Noneguard can therefore pass while the loop isalready closed, and the following
call_soon_threadsaferaises.Fix: snapshot
self.loopinto a local, also guard onloop.is_closed(), and swallow theRuntimeErrorif the worker closes the loop after the check. The
is_closed()guard is exercised by the existingtest_thread_already_stopped; the race-onlyexceptis marked# pragma: no cover.