Skip to content

connection_lost() tears down the worker loop before notifying the application, so disconnect() races the teardown #745

Description

@jetliuzhe

Summary

All the current work on ThreadsafeProxy teardown (#722, #727, #730, #739, #740, #741)
makes the failure graceful — return an awaitable, swallow the RuntimeError, log a
warning. None of it addresses why the proxy is being used against a dead loop in the first
place, which is an ordering problem one layer up in uart.py.

Gateway.connection_lost() resolves connection_done — which triggers
EventLoopThread.force_stop()before it notifies the application that the connection
was lost:

def connection_lost(self, exc):                           # uart.py:67
    ...
    if self._connection_done_future:
        self._connection_done_future.set_result(exc)      # :82  -> fires the :148 callback -> force_stop()
        self._connection_done_future = None
    ...
    self._api.connection_lost(exc)                        # :89  -> only now does the app find out
connection_done.add_done_callback(lambda _: thread.force_stop())   # :148

So the teardown of the loop is already in flight while :89 is still propagating upward
through EZSP.connection_lostControllerApplication.connection_lost → the
application's listener. By the time the application unwinds and calls
await self._ezsp.disconnect(), it is racing that teardown — and disconnect() has to go
through the proxy bound to exactly the loop being torn down.

Making the proxy fail gracefully turns a hang into a no-op, which is a real improvement,
but the disconnect still does not actually run. Flipping the order — notify the
application first, resolve connection_done after — would let the normal cleanup path
complete before the loop goes away, and would make the proxy's disconnected branches a
genuine edge case rather than the common path.

This hazard is in fact already documented a few lines above, for a different future
(uart.py:74-77):

# XXX: The startup reset future must be resolved with an error *before* the
# "connection done" future is completed: the secondary thread has an attached
# callback to stop itself, which will cause the a future to propagate a
# `CancelledError` into the active event loop, breaking everything!

That is the same mechanism — "the secondary thread has an attached callback to stop
itself" — recognised for _startup_reset_future but not for the application notification
at :89, which is also downstream of the same set_result at :82. Given that comment,
I may well be missing a constraint that forces the current order.

I am also fairly confident this is the original bug behind
home-assistant/core#88202 (2023), whose workaround zigpy/zha#628 removed in January
2026 on the assumption that "the underlying bellows issue has been resolved" — while the
PR author notes in the same thread that the original issue was never actually found.
uart.py has been heavily refactored since 2023 (407 → 151 lines), but this ordering has
survived every refactor untouched
: the force_stop done-callback sat at :404 in
Feb 2023 (95c1bee) and sits at :148 today, with set_result(exc) still ahead of the
application notification in both.

Versions

reproduced on also verified present in
bellows 0.49.0 1.0.0 (uart.py byte-identical; thread.py ThreadsafeProxy unchanged)
zigpy 1.2.2 2.1.0
zha 1.1.1 2.1.0 (CONF_USE_THREAD declared at application/const.py:76, zero references)
homeassistant 2026.4.1 dev (only components/zha/radio_manager.py:210 sets it, on the start_radio=False probe app)

Coordinator: EFR32MG24 NCP reached over a raw TCP byte bridge (socket://<ip>:6638).
The bridge does no ASH framing, so nothing on my side interferes with the protocol.

All line numbers in this issue are from bellows 1.0.0 / dev.

What the application then hits

When the application unwinds and calls EZSP.disconnect():

async def disconnect(self):                  # ezsp/__init__.py:231
    self.stop_ezsp()
    if self._gw:
        await self._gw.disconnect()          # _gw is ThreadsafeProxy(gateway, worker_loop)

ThreadsafeProxy.func_wrapper (thread.py:96-107) then dispatches to the worker loop.
Depending on exactly how far force_stop() has progressed, there are three outcomes:

worker loop state is_closed() run_coroutine_threadsafe result tracked in
closed True not reached returns bare Noneawait NoneTypeError #722, #730
closed mid-dispatch False then closes raises RuntimeError exception at caller #740, #741
stopped, not yet closed False does not raise future never resolves → unbounded hang not covered

The third row is the one I hit, and it is the worst of the three because it is completely
silent — no exception, no log line, no retry. force_stop() schedules loop.stop();
loop.close() only happens afterwards in _thread_main's finally, so there is a real
window where the loop is unusable but reports is_closed() == False.

Self-contained reproduction of that window (Python 3.14, no bellows import required):

import asyncio, threading, time, contextlib

loop = asyncio.new_event_loop()
threading.Thread(target=lambda: (asyncio.set_event_loop(loop), loop.run_forever()),
                 daemon=True).start()
time.sleep(0.3)

loop.call_soon_threadsafe(loop.stop)          # what force_stop() does
time.sleep(0.3)
print("is_closed:", loop.is_closed())         # False -> the guard lets it through

async def victim(): return "ran"

fut = asyncio.run_coroutine_threadsafe(victim(), loop)   # does NOT raise
try:
    print("got:", fut.result(timeout=3))
except TimeoutError:
    print("HANG: never resolves; caller has no timeout")

with contextlib.suppress(Exception):
    loop.close()
try:
    asyncio.run_coroutine_threadsafe(victim(), loop)
except RuntimeError as e:
    print("after close:", e)
is_closed: False
HANG: never resolves; caller has no timeout
after close: Event loop is closed

I have also left this as a comment on #741, since that PR states it closes out the last
members of this family and this variant slips past both its is_closed() check and its
except RuntimeError.

Evidence

The hang leaves a distinctive trace when the loop is finally closed and the un-run
coroutine is garbage collected:

WARNING (bellows.thread_0) [py.warnings] /usr/local/lib/python3.14/asyncio/base_events.py:744:
RuntimeWarning: coroutine 'SerialProtocol.disconnect' was never awaited
  self._ready.clear()

base_events.py:744 is inside BaseEventLoop.close(): the coroutine object was still
unwrapped when the loop was closed.

To be precise, this warning on its own does not distinguish the hang from the
RuntimeError variant — in both cases call() has already constructed the coroutine and
it is later collected unused (my reproduction above emits it on both paths). What
identifies the hang in my case is what came with it: no exception anywhere, no
Retrying in … from config_entries, and ss -tn showing zero connection attempts from
the host for the full 90 s I sampled. The RuntimeError and TypeError variants both
surface an exception to the caller; this one surfaced nothing at all.

The TypeError variant has been reported by users as "NoneType errors", e.g.
home-assistant/core#147509 (SLZB-06 Ethernet coordinator), and is already tracked in #722.

Why USB coordinators appear unaffected

The code path is identical, but a USB serial device essentially never raises
connection_lost during normal operation, so this path is not exercised. Network
coordinators hit it on every disconnect — cable, reboot, power loss, DHCP change.

Reproduction

  1. EZSP coordinator over socket://, use_thread left at its default (True).
  2. Cut power to the coordinator (do not close the TCP connection gracefully — a hard
    power cut is the harsher and more realistic case).
  3. Frequently — but not always — the application never reconnects. In Home Assistant the
    config entry never leaves unload; no retries are scheduled and, in the hang variant,
    nothing is logged at all.

Across the reboots I logged before finding the cause, the coordinator came back on its own
5 times out of 9. I would not read too much into the exact ratio (small sample, and I was
not controlling for which of the three variants fired), but it matches the intermittency
described in #88202 and reported by SLZB-06 users.

Verification that use_thread=False still fixes it

With use_thread=False, _connect() runs on the caller's loop, so
ThreadsafeProxy._obj_loop is the main loop, :100 (loop == curr_loop) is hit first,
and :102/:107 are unreachable. EventLoopThread is never created, so there is no
force_stop() at all.

Measured on the setup above (power cut, nothing touched by hand):

event t delta
power cut 17:59:15
connection loss detected, first retry scheduled 17:59:34 19 s
backoff Retrying in 5 s / 10 s 17:59:34 / :45
coordinator back up ≈17:59:50
application reconnected on its own ≈17:59:58 ≈43 s total

No never awaited warning, no failed unload. Same test with the default
use_thread=True is what produced the trace quoted above.

Suggested fix

The minimal change is ordering: notify the application before the
connection_done future is resolved, so the teardown callback at :148 cannot run
until the upper layers have finished with the proxy — extending the invariant that the
XXX comment quoted above already applies to _startup_reset_future.

To be clear about what I am not proposing: removing the UART thread outright was
already tried in #598 and reverted in #604, because serial-port read notifications depend
on the event loop and any loop slowdown stalls radio traffic. That reasoning is sound and
I am not asking to revisit it — the bug here is the teardown ordering, which is
orthogonal to whether the thread exists.

Alternatives, if the ordering is load-bearing for something I have missed:

  • Have EZSP.disconnect() short-circuit when the gateway's loop is no longer usable,
    instead of awaiting through the proxy — at minimum, ThreadsafeProxy.func_wrapper
    should not return a bare None from a coroutine call site (:105), since that turns
    into a confusing TypeError at the await.
  • Give the run_coroutine_threadsafe path at :107 a timeout so a dead loop degrades
    to an error rather than an unbounded hang.
  • Failing all of the above, restore the use_thread=False default for socket://
    device paths that zigpy/zha#628 removed. Note this is narrower than Remove UART thread #598: Revert "Remove UART thread (#598)" #604's
    objection is specifically about serial fd read notifications, which does not apply to
    a TCP transport with kernel-side buffering. It is not free either — it moves ASH ACK
    timing onto the main loop, which is the same failure mode described in Prevent task cancellation from propagating to ASH #628's comments
    about the 2026.1.0 VAD CPU regression — but as a fallback it is strictly better than the
    current unbounded hang.

Happy to test any patch against the reproduction above.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions