Skip to content
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

emontnemery
Copy link
Contributor

@emontnemery 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.

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 changed the title WIP - Fix race in entity_platform.async_add_entities Fix race in entity_platform.async_add_entities Nov 16, 2018
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants