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.

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

Fix race in entity_platform.async_add_entities #18445

Merged
merged 1 commit into from Nov 19, 2018

Conversation

Projects
None yet
3 participants
@emontnemery
Contributor

emontnemery commented Nov 13, 2018

Description:

There is a race in entity_platform.async_add_entities() which makes the check for duplicated entity_id fail. This in turns may cause several entities to be created with the same entity_id.
The issue can very easily be reproduced by publishing persistent MQTT discovery messages with same name before starting hass.

This has previously been reported in #13316 and #15731, although with a slightly different twist
The root cause is described in some detail in #13316

This PR solves the race when there are multiple concurrent calls to entity_platform.async_add_entities() from the same platform.

Possible further improvement:

  • Also detect multiple multiple concurrent calls to entity_platform.async_add_entities() from different platforms as pointed out by @OttoWinter
    Maybe this can be fixed by:
    • Add a lock to not allow concurrent calls to entity_platform.async_add_entities() - dead simple, but might have a negative performance impact
    • Add a dictionary reserved_entity_ids which will hold an entity_id until the entity has been added to hass.states
    • Format the entity_id generated by entity_registry.async_generate_entity_id to also include platform name, e.g. mqtt_name instead of name in the same way as is already done by homeassistant.helpers.entity.async_generate_entity_id

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@emontnemery emontnemery requested a review from home-assistant/core as a code owner Nov 13, 2018

@wafflebot wafflebot bot added the in progress label Nov 13, 2018

Avoid race in entity_platform.async_add_entities()
This avoids a race between multiple concurrent calls to
entity_platform.async_add_entities() which may cause
entities to be created with non-unique entity_id

@emontnemery emontnemery force-pushed the emontnemery:async_add_entities_race branch from 8edf6ee to 8e1a75d Nov 16, 2018

@emontnemery emontnemery changed the title from WIP - Fix race in entity_platform.async_add_entities to Fix race in entity_platform.async_add_entities Nov 16, 2018

@balloob

Nice fix! I think that we should make a better "ensure_unique_string" as there is no reason to chain these things, if anything we should use a ChainMap.

@balloob balloob merged commit f241bec into home-assistant:dev Nov 19, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0002%) to 93.056%
Details

@wafflebot wafflebot bot removed the in progress label Nov 19, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Nov 19, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant:
  Bumped ghlocalapi to 0.1.0 (home-assistant#18584)
  Prefix all xiaomi_aqara events (home-assistant#17354)
  Fix MQTT async_added_to_hass (home-assistant#18575)
  Add mikrotik SSL support (home-assistant#17898)
  Reconfigure MQTT binary_sensor component if discovery info is changed (home-assistant#18169)
  Darksky: Expose missing conditions for day 0 forecast (home-assistant#18312)
  Update pyhomematic to 0.1.52 and add features for lights (home-assistant#18499)
  Fix for epson state not updating (home-assistant#18357)
  Support for Point component (home-assistant#17466)
  Template binary sensor to not track all state changes (home-assistant#18573)
  Correct cached stale device tracker handling (home-assistant#18572)
  Add support for sessions (home-assistant#18518)
  Remove turn_on and turn_off feature for clients (home-assistant#18234)
  Log delay and wait_template steps in scripts (home-assistant#18448)
  Logbook speedup (home-assistant#18376)
  Avoid race in entity_platform.async_add_entities() (home-assistant#18445)
  Fix small issue related to topic prefix (home-assistant#18512)
  Enable native support + ADB authentication for Fire TV (home-assistant#17767)
  Re-adding the season attribute (home-assistant#18523)

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@emontnemery emontnemery deleted the emontnemery:async_add_entities_race branch Dec 2, 2018

@emontnemery emontnemery referenced this pull request Dec 12, 2018

Open

Fix race in entity_platform.async_add_entities #19222

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment