Handle async proxy calls after event loop closes - #730
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #730 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4181 4191 +10
=======================================
+ Hits 4162 4172 +10
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Thanks for tracking this down — the bug is real and I reproduced it against dev.
On dev, ThreadsafeProxy.func_wrapper is a sync function, so for a coroutine method on a closed loop it falls into the # Disconnected branch and returns a bare None. The only in-repo caller that hits this is EZSP.disconnect() (bellows/ezsp/__init__.py:224, await self._gw.disconnect() — disconnect is async via zigpy.serial.SerialProtocol), which then raises TypeError: 'NoneType' object can't be awaited. Confirmed with a minimal repro on cd3378f:
Attempted to use a closed event loop
RAISED: TypeError 'NoneType' object can't be awaited
Splitting the coroutine path into its own async def wrapper fixes that cleanly, and the whole suite passes on the PR head (425 passed, bellows/thread.py at 99% coverage).
One substantive thing I'd like a maintainer call on before this lands, plus two nits — details inline.
The return None is now a silent success for every async proxy method, not just disconnect(). disconnect() genuinely wants a no-op, but reset() and send_data() share the wrapper and now report success on a dead loop. That trades a loud (if ugly) TypeError for a quiet wrong-state. Worth deciding deliberately rather than inheriting it from the sync path's no-op convention — see the inline comment on bellows/thread.py:102.
Not a problem, but worth recording: the refactor also changes when the call is scheduled. On dev the wrapper was sync, so proxy.some_async_method(...) called run_coroutine_threadsafe immediately and returned a Future; now it returns a coroutine and nothing runs until it's awaited. I verified this empirically against a live EventLoopThread (dev: scheduled-without-await True, return type Future; PR: False, return type coroutine). I checked all four in-repo call sites — wait_for_startup_reset (ezsp/__init__.py:124), reset (:165), disconnect (:224), send_data (ezsp/protocol.py:124) — and every one awaits immediately, so nothing regresses today. It does mean a future fire-and-forget call site would silently never run, and the returned object no longer supports Future APIs (add_done_callback, cancel, asyncio.wait).
A second opinion from GitHub Copilot (GPT-5.5) was run on this PR; it independently flagged the same closed-loop silent-success concern, which I then verified against the code.
(Unrelated and pre-existing: the stopped-but-not-yet-closed window in EventLoopThread.force_stop — between loop.stop() and _thread_main's finally: self.loop.close() — still passes is_closed(), so run_coroutine_threadsafe schedules a callback that never runs and the await hangs with no timeout. Same on dev; not something this PR needs to solve.)
| if loop.is_closed(): | ||
| # Disconnected | ||
| LOGGER.warning("Attempted to use a closed event loop") | ||
| return None |
There was a problem hiding this comment.
This makes a closed loop a silent success for all async proxy methods, which is broader than the bug being fixed.
EZSP.disconnect() wants exactly this no-op, but two other callers share the wrapper and now get a false success:
EZSP.reset()(bellows/ezsp/__init__.py:162-169) doesawait self._gw.reset()and then unconditionally callsself.start_ezsp(). WithNonereturned, EZSP is marked running even though no reset frame was ever sent.ProtocolHandler.command()(bellows/ezsp/protocol.py:124) doesawait self._gw.send_data(data)and thenasync with asyncio_timeout(EZSP_CMD_TIMEOUT): return await future. The frame was never written, but the caller now blocks for the full command timeout instead of failing immediately.
On dev both of those raised TypeError right away — noisy, but at least the failure surfaced at the failing operation. In practice all of this happens on the teardown path, so the impact is probably small, but it's a deliberate choice worth making rather than inheriting from the sync path's no-op convention.
One option: raise a ConnectionResetError (or a zigpy ControllerException) here instead, and let EZSP.disconnect() suppress it — that keeps disconnect() idempotent while reset()/send_data() still fail loudly. Entirely your call, though; if the blanket None is the intent, a short comment saying so would help the next reader.
|
|
||
| if asyncio.iscoroutinefunction(func): | ||
|
|
||
| async def async_func_wrapper(*args, **kwargs): |
There was a problem hiding this comment.
Behaviour note for the record — no change requested.
Making this an async def moves the run_coroutine_threadsafe call from call time to await time. Previously proxy.some_async_method(...) scheduled the coroutine on the worker loop immediately and handed back a Future; now it hands back a coroutine that does nothing until awaited.
All four in-repo call sites await immediately, so nothing breaks today. But it does mean:
- a future fire-and-forget call (
proxy.send_data(x)with noawait) would silently never run, and only surface as aRuntimeWarning: coroutine ... was never awaited; - the return value is no longer a
Future, soadd_done_callback,cancel, and bareasyncio.wait()no longer work on it.
Probably worth a one-line docstring or comment on the wrapper so the eager-vs-lazy distinction isn't rediscovered later.
| ) | ||
| ) | ||
|
|
||
| if asyncio.iscoroutinefunction(func): |
There was a problem hiding this comment.
Optional drive-by, since this line is being moved anyway: asyncio.iscoroutinefunction is deprecated and slated for removal in Python 3.16. It's already emitting a DeprecationWarning in the test suite on this file:
bellows/thread.py:91: DeprecationWarning: 'asyncio.iscoroutinefunction' is deprecated and
slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead
I checked that inspect.iscoroutinefunction is a drop-in for every callable shape this proxy sees — bound async def methods (the real case, e.g. Gateway.send_data), plain async def, AsyncMock, and MagicMock attributes all agree between the two. Pre-existing, so feel free to leave it for a separate PR.
| proxy = ThreadsafeProxy(obj, loop) | ||
| loop.close() | ||
|
|
||
| assert await proxy.test() is None |
There was a problem hiding this comment.
The test proves the call is awaitable and yields None, which is the regression that matters. Two small strengthening ideas:
- The sibling
test_proxy_loop_closedassertsobj.test.call_count == 0— i.e. that the target was genuinely not invoked. This test can't, becausetestis a plain function. Wrapping it (obj.test = mock.AsyncMock(wraps=test)or anonlocalcounter) would let you assert the same thing here, which is the other half of "treat it as a disconnected no-op". mock.sentinel.resultis currently unreachable — it only exists to make the assertion meaningful if the guard ever regresses. That's fine, but a counter would make the intent clearer.
Also worth considering: a test pinning the new eager-vs-lazy semantics (that calling without awaiting schedules nothing), so a future refactor back to a sync wrapper doesn't silently flip it.
|
Field report: this reproduces roughly daily on bellows 1.0.0 / HA 2026.8.2, and this PR fixes the cleanup half of it. @puddly asked on #720 for logs from a real installation showing the problem — here is one, on a non-SMLIGHT coordinator. Setup. SONOFF Dongle Max (EFR32MG24 NCP) used as a network coordinator over WiFi, Frequency. An external watchdog polls the ZHA config entry state every 5 minutes. Since 2026-08-11 the entry has been stuck in a non- Latest episode, 2026-09-05 (WARNING level; three of the failure modes discussed in #722 / #740 / #745 appear in one incident): The identical cascade repeated at 16:30:51. The I agree with the point made on core#168432 that the hiccup is the coordinator's doing (the unexpected On this PR. Before finding this thread I had patched the same three sites locally (proxy raises on a closed loop;
Workaround in the meantime, if useful to anyone reading: (Log triage and the bench checks above were done with Claude Code; the installation, the watchdog data and the workaround run are real.) |
3 participants
What changed
ThreadsafeProxyconsistently return an awaitable wrapperWhy
When a threaded EZSP transport loses its connection, the worker event loop can already be closed by the time ZHA asks bellows to disconnect. The existing proxy logs
Attempted to use a closed event loopand returns plainNone, even when the proxied method is asynchronous. The caller then attempts to await that value and raises:This secondary cleanup exception can prevent Home Assistant's ZHA config-entry reload from recovering after the original transport failure.
The async wrapper now remains awaitable in every state. If the target loop is closed, awaiting it returns
None, matching the existing disconnected/no-op behavior without raising a second exception.Related reports:
Validation
test_proxy_async_loop_closedpython -m pytest -q: 425 passedgit diff --check