[Detail Bug] Virtual router outbound KNX frames can fail silently (exceptions swallowed in send path)

Open Beginner friendly
#93 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
86/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
python
Domain
networking

Research direction

Start in apps/knx-gui/src/knx_gui/plugins/virtual/virtual_router.py at VirtualRouter.send_cemi, then compare its behavior with the send_cemi implementation and _log_send_cemi_result in plugins/connection/service.py. Confirm the returned future is observed for failures and that the stopped-router path is handled consistently; use the VirtualService._handle_routing_cemi entry point to understand the call path.

Written by the indexing model from the issue text.

Description

Detail Bug Report

https://app.detail.dev/org_62aa40f5-2c23-4914-a665-3bb2068af20e/bugs/bug_5f3fdb59-8fe1-4c07-8ae8-ab8e949338ce

Introduced in 7686ecbbeb88bd515152379db9ea61a4092a89b5 by @kewde on Jul 21, 2026

Summary

  • Context: VirtualRouter.send_cemi (apps/knx-gui/src/knx_gui/plugins/virtual/virtual_router.py:203-206) is the outbound path for every CEMI frame the virtual device emits on the multicast routing bus. It's invoked by VirtualService._handle_routing_cemi (service.py:91-92) for every reply VirtualDevice.handle_cemi produces — i.e. every Read/Write response a virtual device sends back to the network goes through this one method.
  • Bug: send_cemi discards the concurrent.futures.Future returned by asyncio.run_coroutine_threadsafe(...), so any exception from Routing.send_cemi (OSError on a closed multicast socket, a frame-encode error, or a raise out of the self-echo callback) is swallowed totally silently — no log, no self._error, no ERROR state, no return value to the caller. There is not even a GC-time warning: a discarded concurrent.futures.Future does not emit "Future exception was never retrieved" (that message comes from asyncio.Future/Task, not concurrent.futures.Future); verified empirically under CPython 3.12 (the app's interpreter) — deleting the future and gc.collect()-ing prints nothing.
  • Actual vs. expected: The sibling connection/service.py:82-121 implements the same send_cemi operation with exactly the shape this path is missing — a self._log.warning("send_cemi called while disconnected") early-return and a future.add_done_callback(self._log_send_cemi_result) that logs error=str(exc) on failure (connection/service.py:114-121). VirtualRouter.send_cemi should follow that precedent; instead it returns None and drops the future.
  • Impact: This is a latent defect on a code path no shipping feature exercises — plugin.py wires Start/Stop only to the gateway (start_gateway/stop_gateway), and VirtualService's docstring (service.py:18-21) states the router is "kept around and fully functional, just not started by anything yet." No end user can observe a lost reply from the UI today. The bug is real but its severity should be triaged as "fix when the router is wired up" (or opportunistically now, since the fix is one add_done_callback and matches an in-repo template). A maintainer wiring the router to the UI without touching send_cemi will inherit a path that looks successful on every reply that actually fails to transmit.

Code with Bug

def send_cemi(self, cemi: CEMIFrame) -> None:
    if self._loop is None or self._routing is None:
        return                                       # stop-race guard; reachable only as a race during stop()
    asyncio.run_coroutine_threadsafe(self._routing.send_cemi(cemi), self._loop)
    # ^^^ BUG 🔴 returned Future is discarded (exceptions become totally silent)

Explanation

asyncio.run_coroutine_threadsafe(...) returns a concurrent.futures.Future that holds any exception raised by the coroutine (Routing.send_cemi). Because VirtualRouter.send_cemi discards that Future and does not attach a done-callback (or otherwise inspect .exception()), failures like OSError on send or frame-encoding errors are never observed: no logging, no state transition, and no propagation to callers.

Codebase Inconsistency

apps/knx-gui/src/knx_gui/plugins/connection/service.py performs the same operation but logs the disconnected early-return and attaches a done-callback that logs failures:

def send_cemi(self, raw_cemi: bytes) -> Future[Any] | None:
    if self._xknx is None:
        self._log.warning("send_cemi called while disconnected")
        return None
    ...
    future = self.run_async(self._xknx.knxip_interface.send_cemi(cemi))
    if future is not None:
        future.add_done_callback(self._log_send_cemi_result)
    return future

def _log_send_cemi_result(self, future: Future[Any]) -> None:
    if future.cancelled():
        return
    exc = future.exception()
    if exc is not None:
        self._log.error("send_cemi failed", error=str(exc))
    else:
        self._log.debug("send_cemi ok")

Recommended Fix

Mirror the connection/service.py pattern: keep the returned Future and attach a done-callback that logs success/failure (and optionally log the early-return when the router is not running):

def send_cemi(self, cemi: CEMIFrame) -> None:
    if self._loop is None or self._routing is None:
        if self._logger:
            self._logger.warning("send_cemi called while router not running")
        return
    fut = asyncio.run_coroutine_threadsafe(self._routing.send_cemi(cemi), self._loop)
    fut.add_done_callback(self._on_send_done)

History

This bug was introduced in commit 7686ecb. The commit added L_Data (CEMI) routing to VirtualRouter by wrapping xknx.io.routing.Routing and exposing a send_cemi method so a caller on a non-event-loop thread could push frames onto the multicast bus via asyncio.run_coroutine_threadsafe(..., self._loop). The author verified the success path end-to-end (the commit message describes confirming a frame sent on one VirtualRouter instance is received on another) but added no failure observability by analogy: the returned concurrent.futures.Future was discarded from the very first line of the method, and the early return for an unstarted/stopping router was silent from the start — the sibling connection/service.py precedent (_log.warning + add_done_callback logging error=str(exc)) already existed in the tree but was not mirrored here. git log -L 203,206 and git log -S "run_coroutine_threadsafe(self._routing.send_cemi" both confirm 7686ecb is the sole commit touching these lines: the method has never been modified since its introduction.

Dominant language
Python
Stars
4
Forks
0
Avg merge
17h 43m
Merged PRs (30d)
39

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from XKNX/xknxtoolkit

All issues in XKNX/xknxtoolkit

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.