Fix unloading an entry can leave states around by balloob · Pull Request #17786 · home-assistant/core

balloob · 2018-10-25T09:50:22Z

Description:

Fix for #17370.

The issue is caused by the following (analysis by @amelchio):

Load a config entry that forwards to a platform that adds at least 1 entity that has a unique ID and so is entered into the entity registry
Remove config entry
Component unload will forward unload to platform
Entity Platform correctly unloads the entities from the state machine
When unload succeeds, config entry is removed
Removal of config entry triggers a cleanup on the entity registry to reset the config entry ID of any registered entity with that config entry ID (because the ID will no longer exist). Because this update mutates the entity registry, the attached entity is notified by calling Entity.async_registry_updated.
Entity.async_registry_updated will cause async_update_ha_state to be called, writing the state of a removed entity to the state machine.

Related issue (if applicable): fixes #17370

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for `configuration.yaml` (if applicable):

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

balloob · 2018-10-25T10:19:30Z

CC @amelchio @bachya - could you guys try to repro the bug with the fix from this PR applied ?

I was unable to repro the bug with the extended test case (see diff), but I do have a solution. The problem was that we were not calling the on remove listeners for an entity when it was removed via a platform, because there was some duplicate code. I have fixed that by making the Entity.async_remove method unaware of entity platform and have entity platform get notified of removal via async_on_remove listener.

balloob · 2018-10-25T10:22:44Z

The cleanup will make sure that the entity will no longer be registered for updates on the entity reigstry when the config entry ID is cleared inside the entity registry.

Still bugs me that I was unable to repro it with a test.

MartinHjelmare

I don't have a better suggestion at the moment.

MartinHjelmare · 2018-10-25T10:24:25Z

homeassistant/components/alarm_control_panel/simplisafe.py


    async def async_will_remove_from_hass(self) -> None:
        """Disconnect dispatcher listener when removed."""
+        await super.async_will_remove_from_hass()


I don't like this change. It puts a burden on each platform to make sure the core operates correctly.

Is super.bla correct syntax? I thought it should be super().bla.

good point. will fix.

What about c4a52dd

Just as one opinion: calling super() on methods that subclasses override is just part of Python that needs to be implemented correctly. Just like we have to trust that async platforms aren't doing IO on the main thread, it's OK to trust that subclasses need to be implemented correctly, or they won't work properly.

bachya

@balloob I tested both of my config entry-enabled components (OpenUV and SimpliSafe) against this PR: in both cases, the entities were correctly removed when the config entry was deleted.

From my vantage point, this PR is good to go. 👍

amelchio · 2018-10-25T15:20:28Z

This works with my work-in-progress LIFX unloading as well.

The entities are not removed from group.all_lights but that is probably a separate issue.

amelchio · 2018-10-25T15:55:02Z

Your test never reaches Entity.async_registry_updated because the listener weakref is broken during unloading. I added a commit that makes the test fail without your fix.

balloob · 2018-10-25T17:57:57Z

Awesome detective work guys 👍 Good catch on the test too @amelchio

❤️

* Add test that tests unloading on remove * Add more test things * Untangle entity remove code from entity platform * Don't add default implementation of async_will_remove * Keep entity weakref alive

Add test that tests unloading on remove

d3c115f

homeassistant added the cla-signed label Oct 25, 2018

ghost assigned balloob Oct 25, 2018

ghost added the in progress label Oct 25, 2018

balloob added 2 commits October 25, 2018 12:00

Add more test things

a68e391

Untangle entity remove code from entity platform

fdfb7c2

balloob requested review from a team, Kane610 and bachya as code owners October 25, 2018 10:14

balloob changed the title ~~WIP: Fix unloading an entry can leave states around~~ Fix unloading an entry can leave states around Oct 25, 2018

MartinHjelmare reviewed Oct 25, 2018

View reviewed changes

balloob mentioned this pull request Oct 25, 2018

Restore states through a JSON store instead of recorder #17270

Merged

2 tasks

Don't add default implementation of async_will_remove

c4a52dd

balloob force-pushed the entry-unload-stale-state branch from 689da70 to c4a52dd Compare October 25, 2018 10:35

bachya approved these changes Oct 25, 2018

View reviewed changes

Keep entity weakref alive

ec81003

ghost assigned amelchio Oct 25, 2018

bachya added the Hacktoberfest label Oct 25, 2018

balloob added this to the 0.81 milestone Oct 25, 2018

balloob merged commit 77bf10e into dev Oct 25, 2018

ghost removed the in progress label Oct 25, 2018

balloob deleted the entry-unload-stale-state branch October 25, 2018 17:58

balloob added the cherry-picked label Oct 26, 2018

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

Uh oh!

Conversation

balloob commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

balloob commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob commented Oct 25, 2018

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

balloob Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

balloob Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

emlove Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

bachya left a comment

Choose a reason for hiding this comment

Uh oh!

amelchio commented Oct 25, 2018

Uh oh!

amelchio commented Oct 25, 2018

Uh oh!

balloob commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

balloob commented Oct 25, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob commented Oct 25, 2018 •

edited

Loading

MartinHjelmare Oct 25, 2018 •

edited

Loading