Clean up raw device callbacks after quirk wrapping#1795
Clean up raw device callbacks after quirk wrapping#1795TheJulianJES wants to merge 5 commits intozigpy:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a lifecycle issue in ControllerApplication.device_initialized(): when a device is “quirk wrapped” (raw Device replaced by a new BaseCustomDevice instance), callbacks registered on the raw device were not being cleaned up, leading to stale request-listener registrations.
Changes:
- Replace in-place reassignment with
quirked = zigpy.quirks.get_device(device)and storequirkedintoapp.devices. - Call
device.on_remove()on the raw device when quirk wrapping returns a new object. - Add tests covering the quirk-wrapping vs non-wrapping initialization paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
zigpy/application.py |
Cleans up raw device callbacks when quirk wrapping replaces the device instance. |
tests/test_application.py |
Adds regression tests for raw-device cleanup behavior during initialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1795 +/- ##
=======================================
Coverage 99.52% 99.52%
=======================================
Files 64 64
Lines 12930 12932 +2
=======================================
+ Hits 12868 12870 +2
Misses 62 62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a lifecycle/memory-leak issue during device initialization when quirk matching replaces the raw Device instance with a new (quirked) device object, ensuring callback listeners registered on the raw device don’t remain orphaned in the application’s request-listener registry.
Changes:
- Update
ControllerApplication.device_initialized()to remove the raw device’s_req_listenersentry when quirk wrapping returns a different object. - Adjust initialization flow to consistently use the quirked device for
app.devices, DB listener attachment, anddevice_initializedevent emission. - Add tests covering both the quirk-replacement cleanup case and the no-quirk (same object) case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
zigpy/application.py |
Removes raw-device _req_listeners entry when quirk wrapping replaces the instance; switches subsequent initialization steps to the quirked device. |
tests/test_application.py |
Adds regression tests asserting _req_listeners key removal on quirk replacement and no change when no quirk is applied. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DRAFT.
AI summary
When
device_initialized()applies a quirk viaget_device(), the rawDeviceobject is replaced by a newBaseCustomDeviceinstance. However, the raw device's registered callback listeners (notably the PollControl check-in listener fromDevice.__init__) were never cleaned up, leaving an orphaned_req_listenersentry keyed by the dead raw device object.This removes the raw device's
_req_listenerskey entirely when quirk wrapping creates a new instance. When no quirk matches (same object returned), nothing is changed.Why
_req_listeners.pop()instead ofdevice.on_remove()device_initialized()runs inside the device's own_initialize_task:create_task()adds the task toself._tasksand registersself._tasks.removeas a done callback.on_remove()callsself._tasks.clear(), emptying the set. When_initialize_taskcompletes moments later, its done callback firesself._tasks.remove(task)on the now-empty set, raisingKeyError._req_listeners.pop(device, None)directly removes the orphaned listener entry without touching tasks, avoiding this issue.Related
Related PR that added the
PollControlcheck-in listener:Found here: