Skip NextBus update if integration is still loading #123564
Conversation
home-assistant
bot
added
bugfix
cla-signed
integration: nextbus
small-pr
|
Hey there @ViViDboarder, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| @@ -52,6 +52,10 @@ async def _async_update_data(self) -> dict[str, Any]: | |||
| """Fetch data from NextBus.""" | |||
| self.logger.debug("Updating data from API. Routes: %s", str(self._route_stops)) | |||
|
|
|||
| if self.hass.state is not CoreState.running: | |||
There was a problem hiding this comment.
It seems quite wierd that this integration would be dependent on HA startup (are we fixing a symptom?).
However it would be better to use from homeassistant.helpers.start import async_at_started in __init__.py to delay the startup of this integration rather than returning some empty dict here. See example in speedtestdotnet.
There was a problem hiding this comment.
I updated the PR to use this pattern instead. I verified that this resolves the issue on my production HA instance.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
I'd be interested to see the logs of this happening to fully understand why this is happening |
|
Same. As far as I can tell, this module isn't doing anything unusual with the way the coordinator works. Unless maybe there is a limit to the number of HTTP requests executed sequentially in an executor job, but that doesn't seem like waiting on startup would do anything to resolve it. |
|
The log is in the linked issue #123562. The update thread iterates over the |
|
I think we should re-evaluate the logic within nextbus instead. The issue you are experiencing could also happen the moment someone reloads the integration. So I'd rather opt for a more robust solution that would remove this issue completely. @ViViDboarder Why does every agency have its own coordinator as opposed to per entry? |
|
Ah. I see. So they share a coordinator because of the way the previous API was structured where you could fetch multiple predictions in a single call if they shared an agency. That’s not relevant now. However, I was recently informed that we can fetch multiple routes predictions if they share a stop (found via a bug report because when we fetch for a route it returned all for the stop). Refactoring to share a coordinator per Agency-StopID would be the best approach, but will require another change to the client library. All that said each coordinator should only run once though, so there should be no parallel mutations of the set of stops, unless refresh is being triggered after each stop is added, not after completing the entity initializations. Should figure out if that is indeed the cause because a refactor to use stop based coordinators could still expose the same issue. |
|
Another possible patch would be to have the inline function that is being run in the executor and iterating over the stops access the stops via a parameter rather than from the coordinator directly using a shallow copy, that way if the set is mutated (a new route is added) while an update is happening, it will not run into any issue. |
|
Yes you are refreshing after every integration addition |
|
But if there isn't any limit, let's just do not do smart things |
Fixes a race between the loading thread and update thread leading to an unrecoverable error
|
I updated this PR to use a local copy of |
Much better to solve the issue. Some questions still outstanding but that can be handled in follow-up so if you're fine with this PR don't forget to click "Ready for review" 👍 |
|
Were you able to verify this solves the problem? I'm pretty sure it should, but once you've confirmed click "Ready for Review" as @gjohansson-ST said. |
Fixes a race between the loading thread and
update thread leading to an unrecoverable error
Proposed change
The NextBus integration has a race condition at HA instance start between the thread which sequentially loads configured entities and the update thread which iterates through the set of loaded entities and makes NextBus API calls. The integration never recovers from this error, and will never update again until the HA instance is restarted. The likelihood of encountering this race condition increases as you increase the number of entities.
My fix checks the
hassobject state and returns an empty dict if it is not yet running. I have confirmed that this fixes the issue and that all automated tests pass.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_requirements_all.To help with the load of incoming pull requests: