Skip to content

Handle async proxy calls after event loop closes #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Wackymax wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Handle async proxy calls after event loop closes #730

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a33c704
Handle async proxy calls after event loop closes
Wackymax Jul 16, 2026
fe7e0e1
Handle closed async proxy calls explicitly
Wackymax Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions bellows/thread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,23 @@
)
)

if asyncio.iscoroutinefunction(func):

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Check warning on line 91 in bellows/thread.py

View workflow job for this annotation

GitHub Actions / shared-ci / Run tests Python 3.14 

'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

@zigpy-review-bot zigpy-review-bot Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


async def async_func_wrapper(*args, **kwargs):

@zigpy-review-bot zigpy-review-bot Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

loop = self._obj_loop
curr_loop = asyncio.get_running_loop()
call = functools.partial(func, *args, **kwargs)
if loop == curr_loop:
return await call()
if loop.is_closed():
# Disconnected
LOGGER.warning("Attempted to use a closed event loop")
return None

@zigpy-review-bot zigpy-review-bot Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

future = asyncio.run_coroutine_threadsafe(call(), loop)
return await asyncio.wrap_future(future, loop=curr_loop)

return async_func_wrapper

def func_wrapper(*args, **kwargs):
loop = self._obj_loop
curr_loop = asyncio.get_running_loop()
Expand All @@ -98,21 +115,17 @@
# Disconnected
LOGGER.warning("Attempted to use a closed event loop")
return
if asyncio.iscoroutinefunction(func):
future = asyncio.run_coroutine_threadsafe(call(), loop)
return asyncio.wrap_future(future, loop=curr_loop)
else:

def check_result_wrapper():
result = call()
if result is not None:
raise TypeError(
(
"ThreadsafeProxy can only wrap functions with no return"
"value \nUse an async method to return values: {}.{}"
).format(self._obj.__class__.__name__, name)
)

loop.call_soon_threadsafe(check_result_wrapper)

def check_result_wrapper():
result = call()
if result is not None:
raise TypeError(
(
"ThreadsafeProxy can only wrap functions with no return"
"value \nUse an async method to return values: {}.{}"
).format(self._obj.__class__.__name__, name)
)

loop.call_soon_threadsafe(check_result_wrapper)

return func_wrapper
14 changes: 14 additions & 0 deletions tests/test_thread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,20 @@ async def test_proxy_loop_closed():
assert obj.test.call_count == 0


async def test_proxy_async_loop_closed():
loop = asyncio.new_event_loop()
obj = mock.MagicMock()

async def test():
return mock.sentinel.result

obj.test = test
proxy = ThreadsafeProxy(obj, loop)
loop.close()

assert await proxy.test() is None

@zigpy-review-bot zigpy-review-bot Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



async def test_thread_task_cancellation_after_stop(thread):
loop = asyncio.get_event_loop()
obj = mock.MagicMock()
Expand Down
Loading