8000 Fix race condition for sync entities using throttle by balloob · Pull Request #9891 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix race condition for sync entities using throttle #9891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix race condition for sync entities using throttle #9891

wants to merge 2 commits into from

Conversation

balloob
Copy link
Member
@balloob balloob commented Oct 16, 2017

Description:

I noticed that using the bitcoin sensor would lead to an exception on startup. After analyzing it I realized that update_after_add doesn't work as expected. We handle the situation correctly when add_entities is called on an entity component: we update all entities in sequence before passing adding them to Home Assistant.

However, we don't pass add_entities to setup_platform but instead give them schedule_add_entities. This method calls async_schedule_add_entities under the hood. async_schedule_add_entities will update the entities in parallel.

Updating entities in parallel that use a shared data resource with a throttle on the update function leads to updates being done before the actual data has been fetched:

  1. Call update on entity 1. Entity 1 calls shared_data.update() which makes a web request for 5s
  2. Call update on entity 2. Entity 2 calls shared_data.update() which returns immediately because it was called recently. However the web request is not done yet and thus the data has not been fetched.
  3. Entity 2 assumes update is done and writes to state machine. Since data is not fetched, blows up.
  4. shared_data.update() is done for Entity 1 and writes to state machine just fine.

An alternative to this fix would be to update the throttle function to the following logic:

if called recently:
  return None

# This is new
if call in progress:
  wait till call done
  return None

call actual function()

CC @pvizeli

Exception without this fix

2017-10-15 17:21:13 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/local/Cellar/python3/3.6.2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/tasks.py", line 182, in _step
    result = coro.throw(exc)
  File "/Users/paulus/dev/python/home-assistant/homeassistant/helpers/entity_component.py", line 391, in async_process_entity
    new_entity, self, update_before_add=update_before_add
  File "/Users/paulus/dev/python/home-assistant/homeassistant/helpers/entity_component.py", line 212, in async_add_entity
    yield from self.hass.async_add_job(entity.update)
  File "/usr/local/Cellar/python3/3.6.2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/futures.py", line 332, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/local/Cellar/python3/3.6.2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/tasks.py", line 250, in _wakeup
    future.result()
  File "/usr/local/Cellar/python3/3.6.2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/futures.py", line 245, in result
    raise self._exception
  File "/usr/local/Cellar/python3/3.6.2/Frameworks/Python.framework/Versions/3.6/lib/python3.6/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/paulus/dev/python/home-assistant/homeassistant/components/sensor/bitcoin.py", line 130, in update
    self._state = '{0:.1f}'.format(stats.trade_volume_btc)
AttributeError: 'NoneType' object has no attribute 'trade_volume_btc'

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: bitcoin
    display_options:
      - exchangerate
      - trade_volume_btc

@balloob balloob requested a review from a team as a code owner October 16, 2017 00:25
@balloob
Copy link
Member Author
balloob commented Oct 16, 2017

I actually think that waiting for call to end is the more proper fix. That way we will avoid the same race condition from happening during normal update cycles.

@balloob
Copy link
Member Author
balloob commented Oct 16, 2017

Nevermind my last comment. Normal update cycle will already run the updates of synchronous entities in sequence instead of parallel

@balloob
Copy link
Member Author
balloob commented Oct 16, 2017

Probably better to just extract this part as a method and call it during async_add_entities too. Also wondering if we are correctly reporting exceptions that happen in async updates.

@balloob
Copy link
Member Author
balloob commented Oct 19, 2017

Fixed in #9924

@balloob balloob closed this Oct 19, 2017
@balloob balloob deleted the throttle-bitcoin branch October 19, 2017 15:50
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla-signed core Hacktoberfest integration: bitcoin platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@balloob @fabaff @balloobbot @homeassistant
0