Hold a lock to pre 8000 vent concurrent setup of config entries #116482
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If something triggers a config entry to reload while we are still in the setup of the underlying component/integration the lock was not held because async_setup_component does not hold the reload lock. Rename the reload lock to setup lock.
bdraco
commented
Apr 30, 2024
Comment on lines
+1795
to
+1801
| if entry.domain not in self.hass.config.components: | ||
| # If the component is not loaded, just load it as | ||
| # the config entry will be loaded as well. We need | ||
| # to do this before holding the lock to avoid a | ||
| # deadlock. | ||
| await async_setup_component(self.hass, entry.domain, self._hass_config) | ||
| return entry.state is ConfigEntryState.LOADED |
There was a problem hiding this comment.
This is exactly what self.async_setup(entry_id) does, but we need to do it without holding the lock since async_setup_component will hold the lock
joostlek
approved these changes
Apr 30, 2024
|
testing this on my production now. I want to add an explicit test for the case where we have a long setup time and we trigger a reload |
|
I also need to profile this |
|
profile is good pushed to all production |
…t_setup' into hold_lock_to_prevent_concurrent_setup
|
I've beat this up as much as I can on my production and dev envs. |
frenck
pushed a commit
that referenced
this pull request
May 1, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
If something triggers a config entry to reload while we are still in the setup of the underlying component/integration, the lock was not held because
async_setup_componentdoes not hold the reload lock. We only had a lock to prevent concurrent reloads, and there wasn't anything to handle a reload during startup. The common case is that there is a config entry update listener and it triggers a reload because something during its setup updates its config entry data.This has been a problem for a long time but it wasn't fast enough until 2024.5.x for us to see this happen on a regular basis.
config_entries.async_removeshould also probably hold the lock to unload the entry and remove it but I didn't want to make that change in this PR as the problem is far less likely to happenRename the reload lock to setup lock.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requiremen 8000 ts_all..coveragerc.To help with the load of incoming pull requests: