Skip to content

RFC - Add a 3rd state to the HA shutdown sequence for writing data#33358

Merged
balloob merged 7 commits intohome-assistant:devfrom
dmulcahey:dm/fix-zha-tests
Mar 30, 2020
Merged

RFC - Add a 3rd state to the HA shutdown sequence for writing data#33358
balloob merged 7 commits intohome-assistant:devfrom
dmulcahey:dm/fix-zha-tests

Conversation

@dmulcahey
Copy link
Copy Markdown
Contributor

Breaking change

shouldn't be

Proposed change

When investigating the test failures in ZHA exposed by the new exception handling I noticed there is a potential race condition in things that use the storage helper if you leverage the EVENT_HOMEASSISTANT_STOP event to update the data that the storage helper will write out. This PR adds an additional stage to the shutdown sequence so that the data writing can be separated from the stop event. By doing this, anything that needs or wants to update data on stop can do so and it will be written out in the subsequent stage.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

Comment thread homeassistant/core.py Outdated
Comment thread homeassistant/core.py Outdated
@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 29, 2020

So this change makes sense.

One thing that we can consider (in a future PR) is that if a store is asked to write/delay write something while Home Assistant is stopping, that we hold that data until the final write event.

@dmulcahey dmulcahey marked this pull request as ready for review March 29, 2020 21:41
@dmulcahey
Copy link
Copy Markdown
Contributor Author

dmulcahey commented Mar 29, 2020

So this change makes sense.

One thing that we can consider (in a future PR) is that if a store is asked to write/delay write something while Home Assistant is stopping, that we hold that data until the final write event.

how would you do that? Subscribe to stop and set a field to track that stop was called then if you get a save / delay save log (nah that would be noisy) and return so that it's handled by final write?

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 30, 2020

Add to both async_save and async_delay_save something like:

if hass.state == CoreState.stopping:
    self._data = {…}
    self._async_ensure_stop_listener()
    return

@dmulcahey
Copy link
Copy Markdown
Contributor Author

Add to both async_save and async_delay_save something like:

if hass.state == CoreState.stopping:
    self._data = {…}
    self._async_ensure_stop_listener()
    return

Cool. I can tackle that in another PR as suggested or in this one if ya want.

@balloob balloob merged commit bcd1eb9 into home-assistant:dev Mar 30, 2020
@dmulcahey dmulcahey deleted the dm/fix-zha-tests branch April 2, 2020 11:22
@lock lock Bot locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed core small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants