Handle async proxy calls after event loop closes

[Wackymax]

What changed

• make coroutine methods exposed by ThreadsafeProxy consistently return an awaitable wrapper
• treat calls made after the worker event loop has closed as a disconnected no-op
• add a regression test covering an async proxy method after loop closure

Why

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 loop and returns plain None, even when the proxied method is asynchronous. The caller then attempts to await that value and raises:

TypeError: object NoneType can't be used in 'await' expression

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:

• ZHA - application controller is not running home-assistant/core#130548
• Controller doesnt reconnect after serial connection lost. #654

Validation

• added test_proxy_async_loop_closed
• python -m pytest -q: 425 passed
• git diff --check

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (cd3378f) to head (a33c704).

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.
📢 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.

[zigpy-review-bot]

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.)

[zigpy-review-bot]

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) does await self._gw.reset() and then unconditionally calls self.start_ezsp(). With None returned, EZSP is marked running even though no reset frame was ever sent.
• ProtocolHandler.command() (bellows/ezsp/protocol.py:124) does await self._gw.send_data(data) and then async 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.

[zigpy-review-bot]

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 no await) would silently never run, and only surface as a RuntimeWarning: coroutine ... was never awaited;
• the return value is no longer a Future, so add_done_callback, cancel, and bare asyncio.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.

[zigpy-review-bot]

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.

[zigpy-review-bot]

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_closed asserts obj.test.call_count == 0 — i.e. that the target was genuinely not invoked. This test can't, because test is a plain function. Wrapping it (obj.test = mock.AsyncMock(wraps=test) or a nonlocal counter) would let you assert the same thing here, which is the other half of "treat it as a disconnected no-op".
• mock.sentinel.result is 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.