Repository navigation
Require aiosqlite 0.22.1 - #1898
TheJulianJES wants to merge 2 commits into
Conversation
aiosqlite 0.22 no longer makes `Connection` a `Thread`: the worker thread is now `Connection._thread`, `_stop_running()` became `stop()`, and `close()` waits for the worker to process the stop request. Require 0.22.1, which has the public `stop()`, and join the worker thread through `_thread` on shutdown, since `close()` can still return just before the thread exits. `auto_kill_aiosqlite` now tracks the connections created during a test instead of looking for `Connection` threads, and `verify_cleanup` drops its aiosqlite special case, which can no longer match.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1898 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 61 61
Lines 12773 12774 +1
=======================================
+ Hits 12711 12712 +1
Misses 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # `close()` can return before aiosqlite's worker thread has exited | ||
| await asyncio.get_running_loop().run_in_executor( | ||
| None, self._db._thread.join | ||
| ) |
There was a problem hiding this comment.
Still not happy about reaching into internals here but SQLAlchemy's dialect seems to do something similar still: https://github.com/sqlalchemy/sqlalchemy/blob/529fa3520bcc59ffc884ffe5bcb46d7a9961ebd5/lib/sqlalchemy/dialects/sqlite/aiosqlite.py#L293-L299
I'll look into this a bit more. Upstream issue seems to be omnilib/aiosqlite#387
There was a problem hiding this comment.
I was working on an SQLAlchemy port last year, we could also try it: #1637
There was a problem hiding this comment.
Yeah. I was looking at this just because of the previous batching PR and wanting to do some other improvements before playing around with the generic storage stuff. But if we can also move to SQLAlchemy before, that might be better(?)
There was a problem hiding this comment.
I looked into it a bit more, and _thread still looks like the least bad option:
Why (CLICK TO EXPAND)
- There's no public way to wait for the thread. Add a join() method omnilib/aiosqlite#387 has had no response since July, and nothing has landed upstream since 0.22.1 (Dec 2025).
- Awaiting the future that
stop()returns doesn't help either.close()already awaits the one from its ownstop()call, and the worker schedules its result just before leaving its loop. Callingstop()again afterwards returns a new future that never resolves, because the worker is gone. - The join is still needed. Without extra load the thread is rarely still alive when
close()returns: 0 of 600 cycles in one run, 2 of 600 in another on a busy machine. With two CPU-bound threads competing for the GIL and a short switch interval, it was still alive in 10 to 15 of 200. - SQLAlchemy reaches into
connection._threadfor 0.22 too (SqlAlchemy aiosqlite wrapper doesn't close the aiosqlite connection object (hanging thread) sqlalchemy/sqlalchemy#13039, to make it a daemon thread). zzzeek wasn't happy about it either, but merged it. - If aiosqlite ever renames it, the failure is mild: the
AttributeErrorcomes after the connection is already closed,ControllerApplication.shutdown()logs it as a warning, and our shutdown tests that use_threadfail in CI once it rebuilds its environment, so we'd notice.
I pushed 2b752da: the join now runs in its own finally, so it also happens if close() raises. The comment there points to omnilib/aiosqlite#387, and a new test makes aiosqlite's close step fail and checks that join() still runs. Once omnilib/aiosqlite#387 lands, we can switch to the public method.
On the SQLAlchemy port (CLICK TO EXPAND)
On the SQLAlchemy port (#1637, closed as stale): SQLAlchemy still runs on aiosqlite underneath. Its dialect closes connections with a plain await connection.close() and no join, and since 0.22 it avoids hangs at exit by setting connection._thread.daemon = True. So a port moves the private access into SQLAlchemy rather than removing it; the window after close() stays, but the process can still exit because the thread is a daemon. On the ordering question: I don't think the port has to come first. The storage-seam work for the generic store needs a key/value interface routed through the worker, a migration strategy, and lifetimes that don't hang off foreign keys. SQLAlchemy doesn't provide any of those by itself. Where it would help is as a statement builder, or if we want other backends later. If we do port, two things have to be redone: the #1835 commit batching (the timer and forced commits drive commit() on the aiosqlite connection), and the migration rollback that #1637's description says fails. Either way, this PR is small and independent of that decision, and it unblocks 0.22 now.
There was a problem hiding this comment.
@zigpy-review-bot With SQLAlchemy setting connection._thread.daemon = True, would that still trigger HA warnings about leaking threads on shutdown? (Unlike this approach?) If so, we'd also need to work around that if we used SQLAlchemy directly?
There was a problem hiding this comment.
No, the daemon flag wouldn't cause HA warnings. HA has no leaked-thread warning at runtime, and the daemon flag only makes HA's shutdown skip the thread. With this PR it doesn't matter either way, since the thread is joined before ControllerApplication.shutdown() returns.
For a SQLAlchemy port, HA itself wouldn't need a workaround. Our own test suite isn't affected today either: zigpy's verify_cleanup is currently a no-op, because it's a plain sync @pytest.fixture that calls asyncio.get_running_loop(), which raises there, so it returns before checking anything. HA's version is a @pytest_asyncio.fixture using asyncio.get_event_loop(), and zha's is an async fixture. Once ours is fixed the same way, SQLAlchemy's await connection.close() without a join could make the rare window after close() described in the PR body show up as an occasional "Lingering thread" failure, unless a fixture still joins the thread (through _thread again) or verify_cleanup skips aiosqlite's threads.
What I checked (CLICK TO EXPAND)
- HA runtime:
runner.pyreplacesthreading._shutdownwithdeadlock_safe_shutdown()(homeassistant/util/thread.py). That collects threads that are alive and not daemon, joins them within a shared 10 s budget, and only logs ifjoin()itself raises. A daemon thread is skipped; a non-daemon one that is still finishing just gets joined. Neither logs anything. - HA's and zha's test suites: their
verify_cleanupfixtures fail on any new thread after a test except_DummyThreadand waitpid threads (and, in HA,_run_safe_shutdown_loop), so the daemon flag wouldn't exempt anything there. Neither suite opens a zigpy database, though: both setCONF_DATABASE: Nonefor the app controller, so no aiosqlite thread is ever started in them. - Python itself (aiosqlite 0.22.1, Python 3.14): an unclosed connection only lets the process exit if it gets garbage-collected first, because aiosqlite's
__del__then stops the thread. If something still holds a reference at exit, as SQLAlchemy's pool does, plain Python waits on the non-daemon worker thread forever. With_thread.daemon = Trueset before the connection is awaited, it exits, with aiosqlite'sResourceWarning. Under HA,deadlock_safe_shutdown()would cap that wait at 10 s. - What SQLAlchemy's flag does: it keeps an open (or still-finishing) worker thread from holding up interpreter exit. It doesn't make the thread finish any sooner after
close(). It is also only set when the dialect callsaiosqlite.connect()itself, not when anasync_creator_fnis passed.
aiosqlite's `close()` always stops its worker thread, even when closing the connection raises, so the join is now in a `finally` and still runs in that case. The comment also points to omnilib/aiosqlite#387, which asks for a public way to wait for the thread.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The dependency, shutdown implementation, cleanup fixture, and tests consistently support aiosqlite 0.22.1.
Review effort: Balanced
Findings: None
What changed in this PR
Adds compatibility with aiosqlite 0.22.1’s revised worker-thread lifecycle.
Changes:
- Requires aiosqlite 0.22.1 or newer.
- Joins the internal worker thread during shutdown.
- Updates cleanup fixtures and shutdown tests for the new API.
| File | Description |
|---|---|
zigpy/appdb.py |
Joins the new worker thread after closing. |
tests/test_appdb.py |
Updates and extends shutdown coverage. |
tests/conftest.py |
Tracks and stops leaked connections. |
pyproject.toml |
Raises the minimum aiosqlite version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Even if we port to SQLAlchemy next, I think it still makes sense to get this upgrade in before then. I’ll look at everything else. (And note: Both the previous batching PR and this one are not something for the current HA cycle. If we need zigpy bugfixes, we can always release that separately from this.) |
This PR adds support for aiosqlite 0.22.x and also requires 0.22.1 (or newer – currently latest).
See their changelog: https://github.com/omnilib/aiosqlite/blob/HEAD/CHANGELOG.md
Also closes #1762
AI-generated PR description
Lifts the
<0.22.0cap on aiosqlite added in #1729. aiosqlite 0.22 is a breaking release for zigpy:Connectionis no longer aThreadsubclass. The worker thread is nowConnection._thread, and_stop_running()became the publicstop()in 0.22.1. Ondevwith 0.22.1 installed,PersistingListener.shutdown()fails withAttributeError: 'Connection' object has no attribute 'join'.Changes
aiosqlite>=0.22.1. That release (Improve stop semantics for connections omnilib/aiosqlite#370) adds the publicstop(), which also works without a running event loop; the test fixture relies on that. Nothing else in the dependency tree caps aiosqlite: zha and the radio libraries don't depend on it, and the only other requirement in HA Core's tree, knx-telegram-store'saiosqlite>=0.20, is a lower bound.shutdown()joins the worker thread throughself._db._threadinstead ofself._db. The join now sits in its ownfinally, so it also runs ifclose()raises; aiosqlite'sclose()stops the thread in that case too.auto_kill_aiosqlitenow records the connections created during the test, by wrappingConnection.__init__, and callsstop()on any whose thread is still alive. It used to scanthreading.enumerate()forConnectionthreads, which no longer exist.verify_cleanupdrops its aiosqlite special case, sinceisinstance(thread, aiosqlite.Connection)can no longer be true.listener._db._thread.is_alive()instead oflistener._db.is_alive().About the race from #1729: since 0.22.0, which includes omnilib/aiosqlite#305,
await close()returns only after the worker has processed the stop request. The worker schedules that future's result just before it leaves its loop, though, so the thread can still be running whenclose()returns. Without extra load this is rare: 0 of 600 close/check cycles in one run, 2 of 600 in another on a busy machine. With two CPU-bound threads competing for the GIL and a short switch interval, the thread was still alive right afterclose()in 10 to 15 of 200 cycles across three runs. So the explicit join stays.About
_thread: aiosqlite has no public way to wait for the worker thread. omnilib/aiosqlite#387 asks for ajoin()method and has no response yet. Awaiting the future returned bystop()doesn't help either.close()already awaits the future from its ownstop()call, and callingstop()again afterwards returns a new future that never resolves, because the worker has already exited. SQLAlchemy's aiosqlite dialect reaches intoconnection._threadtoo since 0.22 (sqlalchemy/sqlalchemy#13039), to mark it as a daemon thread. If a future aiosqlite release renames the attribute,shutdown()raisesAttributeErroronly after the connection is closed,ControllerApplication.shutdown()logs it as a warning, and the shutdown tests that check_threadfail in CI once it rebuilds its environment. #1762 skipped the join whenjoinwas missing; that would silently bring back the race, and this PR supersedes it.Testing: the full suite passes on aiosqlite 0.22.1 (1350 passed).
test_shutdown_joins_thread_when_close_failsmakes aiosqlite's own close step raise, spies on the thread'sjoin(), and fails if the join is skipped. With an open connection deliberately left behind by a test,auto_kill_aiosqlitestops its thread after the test.