Handle the loop closing between ThreadsafeProxy's is_closed() check and dispatch - #741
Open
zigpy-review-bot wants to merge 3 commits into
Open
Handle the loop closing between ThreadsafeProxy's is_closed() check and dispatch#741zigpy-review-bot wants to merge 3 commits into
ThreadsafeProxy's is_closed() check and dispatch#741zigpy-review-bot wants to merge 3 commits into
Conversation
… and dispatch The worker thread can close the loop after `func_wrapper`'s `is_closed()` check but before `run_coroutine_threadsafe()` / `call_soon_threadsafe()`, raising `RuntimeError: Event loop is closed` at the caller -- the same race #727 suppressed in `force_stop()`. Treat close-during-dispatch like already-closed: log the existing warning and drop the call. Disconnected async proxy calls (both already-closed and closed-mid- dispatch) now return an already-completed future resolving to None instead of bare None, so `await proxy.method()` no longer raises `TypeError: object NoneType can't be used in 'await' expression`. Also raise a legible `RuntimeError` from `EventLoopThread.run_coroutine_threadsafe()` when the loop is already gone, instead of `AttributeError: 'NoneType' object has no attribute 'call_soon_threadsafe'`, closing the passed coroutine so it doesn't warn as never-awaited. Closes #740
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #741 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 64 64
Lines 4263 4286 +23
=======================================
+ Hits 4244 4267 +23
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`asyncio.iscoroutinefunction()` is deprecated since Python 3.14 and slated for removal in 3.16; `inspect.iscoroutinefunction()` is a drop-in replacement for every case here (coroutine functions, plain callables, `functools.partial`, `AsyncMock`).
`EventLoopThread.run_coroutine_threadsafe`'s `None` guard covers the already-exited worker, but when the worker closes the loop after the snapshot, `asyncio.run_coroutine_threadsafe()` raises `RuntimeError` correctly while the passed coroutine leaks as never-awaited. Close it before re-raising, mirroring the proxy's async dispatch path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #740. Follow-up to #727 and #739 — this closes out the last members of the
RuntimeError: Event loop is closedteardown-race family.ThreadsafeProxy.func_wrappersnapshots the loop object at construction, so #739's ordering fix (publishself.loop = Nonebefore closing) can't reach it: the worker thread can close the loop between the proxy'sloop.is_closed()check and the dispatch viaasyncio.run_coroutine_threadsafe()/loop.call_soon_threadsafe(), and both raiseRuntimeError: Event loop is closedat the caller. This wraps both dispatches intry/except RuntimeErrorand treats close-during-dispatch exactly like already-closed: log the existing "Attempted to use a closed event loop" warning and drop the call. Theexceptis narrow — bothtryblocks wrap only the scheduling call (for the async path,call()merely constructs the coroutine; its body never runs there), so noRuntimeErrorfrom user code can be swallowed.Two findings from pre-open review (details below) are folded in:
is_closed()branch returned bareNonefor coroutine methods too, so real callers that await through a proxy (await self._gw.reset(),await self._gw.disconnect(),await self._gw.send_data(...)) gotTypeError: object NoneType can't be used in 'await' expressionwhen disconnected — trading one teardown exception for another. Both disconnected paths (already-closed and closed-mid-dispatch) now return an already-completed future for coroutine methods, soawait proxy.method()resolves cleanly toNone; sync calls still returnNoneas before. On the race path the never-to-be-scheduled coroutine is explicitly closed, so it doesn't warn as never-awaited.EventLoopThread.run_coroutine_threadsafe()raises legibly when the loop is gone. After ClearEventLoopThread.loopbefore closing it #739 the shutdown-window failure there wasAttributeError: 'NoneType' object has no attribute 'call_soon_threadsafe'; it now snapshotsself.loopand raisesRuntimeError("Event loop is not running"). The sole caller (uart.connect) already handles this viaexcept Exception.Two follow-up commits, from post-open review of the same family:
run_coroutine_threadsafe()'s failure paths. TheNoneguard closes it before raising, and when the worker closes the loop after the snapshot (soasyncio.run_coroutine_threadsafe()itself raisesRuntimeError), it is now closed before re-raising too — mirroring the proxy's async dispatch path — so neither failure mode leaks a "coroutine ... was never awaited"RuntimeWarningat teardown.inspect.iscoroutinefunction()replaces the deprecatedasyncio.iscoroutinefunction()(deprecated since Python 3.14, removal slated for 3.16; these were bellows' only two call sites). Drop-in for every case here: real coroutine functions, plain callables,functools.partial,AsyncMock.Tests
Five new tests.
test_proxy_loop_closed_during_async_dispatchandtest_proxy_loop_closed_during_sync_dispatchpin the race window by makingloop.call_soon_threadsaferaiseRuntimeErrorwhileis_closed()still returnsFalse— a faithful stand-in, since a genuinely closed loop would trip the earlier guard.test_proxy_loop_closed_asyncpins the awaitable disconnected result on the already-closed branch;test_thread_run_coroutine_threadsafe_loop_not_runningpins theRuntimeErroron theNonepath;test_thread_run_coroutine_threadsafe_loop_closed_mid_dispatchpins the coroutine being closed (CORO_CLOSED) on the post-snapshot race path. All five fail ondevand pass with the change.Verification
pytest tests/→ 446 passed (Python 3.14.5), clean under-W error::RuntimeWarning -W error::DeprecationWarning(pins both the no-leaked-coroutine behavior and the deprecation removal)bellows/thread.pyreverted todev: the five new tests fail (leakedRuntimeError×2,TypeErroronawait None,AttributeError, leaked never-awaited coroutine), rest passexcept RuntimeErrorbreadth is safe and flagged the coroutine leak on the raise path) and a Copilot CLI second opinion (GPT-5.6 Sol; flagged theawait NoneTypeError) — both findings addressed as describedautoflake,black,flake8,isort,codespell,ruffpass;pyupgrade/mypyfail on untouched files in the local hook env only (same Python 3.14 breakage as noted on ClearEventLoopThread.loopbefore closing it #739) — CI is the real check