Skip to content

Replace asyncio.wait with asyncio.gather since wait ignores exceptions #33380

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

Merged
balloob merged 13 commits into from
Apr 1, 2020
Merged

Replace asyncio.wait with asyncio.gather since wait ignores exceptions #33380

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
de40b61
replace asyncio.wait with asyncio.gather since wait ignores exceptions
ziv1234 Mar 28, 2020
f649a7a
changed to log on failed domains
ziv1234 Mar 30, 2020
f48b612
discovered that this fix actually removes other uncaught exceptions
ziv1234 Mar 30, 2020
265e616
fix in the list of ignored exceptions
ziv1234 Mar 31, 2020
f64a6c4
replaced a few ignores on dyson tests that work locally but fail in t…
ziv1234 Mar 31, 2020
a226537
two more tests that are failing on the CI and not locally
ziv1234 Mar 31, 2020
67f8738
removed assertion on multiple entries with same unique_id - replaced …
ziv1234 Mar 31, 2020
d253ea0
entered all the dyson tests. they all pass locally and probabilistica…
ziv1234 Mar 31, 2020
c3ea753
removed unnecessary str() for exception
ziv1234 Mar 31, 2020
fd3eab6
added log message for duplicate entity_id / unique_id
ziv1234 Mar 31, 2020
323e21f
removed log in case of False return value
ziv1234 Mar 31, 2020
9f62850
added exc_info
ziv1234 Mar 31, 2020
0934ac9
change the use of exc_info
ziv1234 Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 30 additions & 15 deletions homeassistant/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,15 +325,38 @@ async def _async_set_up_integrations(
hass: core.HomeAssistant, config: Dict[str, Any]
) -> None:
"""Set up all the integrations."""

async def async_setup_multi_components(domains: Set[str]) -> None:
"""Set up multiple domains. Log on failure."""
futures = {
domain: hass.async_create_task(async_setup_component(hass, domain, config))
for domain in domains
}
await asyncio.wait(futures.values())
errors = [
domain
for domain in domains
if futures[domain].exception() or not futures[domain].result()
]
for domain in errors:
if futures[domain].exception():
_LOGGER.error(
"Error setting up integration %s - received exception %s",
domain,
futures[domain].exception(),
)
else:
_LOGGER.error(
"Error setting up integration %s - returned False", domain

@balloob balloob Mar 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The entity platform is already dealing with return value of False

@ziv1234 ziv1234 Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, so just remove the log in that case?

@balloob balloob Mar 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah.

@ziv1234 ziv1234 Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

)

domains = _get_domains(hass, config)

# Start up debuggers. Start these first in case they want to wait.
debuggers = domains & DEBUGGER_INTEGRATIONS
if debuggers:
_LOGGER.debug("Starting up debuggers %s", debuggers)
await asyncio.gather(
*(async_setup_component(hass, domain, config) for domain in debuggers)
)
await async_setup_multi_components(debuggers)
domains -= DEBUGGER_INTEGRATIONS

# Resolve all dependencies of all components so we can find the logging
Expand All @@ -358,9 +381,7 @@ async def _async_set_up_integrations(
if logging_domains:
_LOGGER.info("Setting up %s", logging_domains)

await asyncio.gather(
*(async_setup_component(hass, domain, config) for domain in logging_domains)
)
await async_setup_multi_components(logging_domains)

# Kick off loading the registries. They don't need to be awaited.
asyncio.gather(
Expand All @@ -370,9 +391,7 @@ async def _async_set_up_integrations(
)

if stage_1_domains:
await asyncio.gather(
*(async_setup_component(hass, domain, config) for domain in stage_1_domains)
)
await async_setup_multi_components(stage_1_domains)

# Load all integrations
after_dependencies: Dict[str, Set[str]] = {}
Expand Down Expand Up @@ -401,9 +420,7 @@ async def _async_set_up_integrations(

_LOGGER.debug("Setting up %s", domains_to_load)

await asyncio.gather(
*(async_setup_component(hass, domain, config) for domain in domains_to_load)
)
await async_setup_multi_components(domains_to_load)

last_load = domains_to_load
stage_2_domains -= domains_to_load
Expand All @@ -413,9 +430,7 @@ async def _async_set_up_integrations(
if stage_2_domains:
_LOGGER.debug("Final set up: %s", stage_2_domains)

await asyncio.gather(
*(async_setup_component(hass, domain, config) for domain in stage_2_domains)
)
await async_setup_multi_components(stage_2_domains)

# Wrap up startup
await hass.async_block_till_done()
4 changes: 2 additions & 2 deletions homeassistant/helpers/entity_component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ async def async_setup(self, config: ConfigType) -> None:
tasks.append(self.async_setup_platform(p_type, p_config))

if tasks:
await asyncio.wait(tasks)
await asyncio.gather(*tasks)

@balloob balloob Mar 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't want this, because we don't want setup of 1 platform cancel the others. The same with the change for reset. We still want to log them.

@balloob balloob Mar 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In all proposed changes here, it's ok to just log the errors and continue.

@ziv1234 ziv1234 Mar 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, but brings up a few questions / suggestions:

  1. If we don't want this specific exception to be caught, perhaps instead of throwing the exception, just logging? Is there anywhere that we do catch it and do something with it?
  2. If my change passed all the unit tests, but there is functionality that it breaks (we don't want setup of 1 platform cancel the others), perhaps we should add a unit tests that tests this scenario. I don't know enough about the startup process of hass to create this, but sounds like a simple test to prevent others from making my mistake :)
  3. In any case, even if we cancel this exception, probably better to change the wait(tasks) to gather(*tasks) so the exceptions don't disappear and catch them in the calling method. I tried looking and it seems like _async_set_up_integrations in bootstrap.py may be the right place, but not sure

What do you think is the best behavior? Maybe something like:

  • during startup, we log the exceptions as we don't want to fail the other components
  • when an integration is loaded dynamically, leave the exception so it would fail the load
    As mentioned, I am pretty ignorant about the startup process, so hopefully this makes some sense...

@balloob balloob Mar 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. We should log it in this method where we change from wait to gather. Gather can return exceptions.
  2. Yeah that's bad. I'll add one.
  3. Yeah

@ziv1234 ziv1234 Mar 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok. just added the changes to _async_set_up_integrations
I assume CORE_INTEGRATIONS don't need to be checked as if they fail, we fail anyway, right?


# Generic discovery listener for loading platform dynamically
# Refer to: homeassistant.components.discovery.load_platform()
Expand Down Expand Up @@ -263,7 +263,7 @@ async def _async_reset(self) -> None:
tasks.append(platform.async_destroy())

if tasks:
await asyncio.wait(tasks)
await asyncio.gather(*tasks)

self._platforms = {self.domain: self._platforms[self.domain]}
self.config = None
Expand Down
13 changes: 7 additions & 6 deletions homeassistant/helpers/entity_platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ async def _async_setup_platform(self, async_create_setup_task, tries=0):
self._tasks.clear()

if pending:
await asyncio.wait(pending)
await asyncio.gather(*pending)

hass.config.components.add(full_name)
return True
Expand Down Expand Up @@ -292,7 +292,7 @@ async def async_add_entities(
if not tasks:
return

await asyncio.wait(tasks)
await asyncio.gather(*tasks)

if self._async_unsub_polling is not None or not any(
entity.should_poll for entity in self.entities.values()
Expand Down Expand Up @@ -431,10 +431,11 @@ async def _async_add_entity(
already_exists = True

if already_exists:
msg = f"Entity id already exists: {entity.entity_id}"
msg = f"Entity id already exists - ignoring: {entity.entity_id}"
if entity.unique_id is not None:
msg += f". Platform {self.platform_name} does not generate unique IDs"
raise HomeAssistantError(msg)
self.logger.error(msg)
return
Comment thread
MartinHjelmare marked this conversation as resolved.
Outdated

entity_id = entity.entity_id
self.entities[entity_id] = entity
Expand All @@ -459,7 +460,7 @@ async def async_reset(self) -> None:

tasks = [self.async_remove_entity(entity_id) for entity_id in self.entities]

await asyncio.wait(tasks)
await asyncio.gather(*tasks)

if self._async_unsub_polling is not None:
self._async_unsub_polling()
Expand Down Expand Up @@ -548,7 +549,7 @@ async def _update_entity_states(self, now: datetime) -> None:
tasks.append(entity.async_update_ha_state(True)) # type: ignore

if tasks:
await asyncio.wait(tasks)
await asyncio.gather(*tasks)


current_platform: ContextVar[Optional[EntityPlatform]] = ContextVar(
Expand Down
2 changes: 1 addition & 1 deletion tests/helpers/test_entity_platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ async def test_adding_entities_with_generator_and_thread_callback(hass):

def create_entity(number):
"""Create entity helper."""
entity = MockEntity()
entity = MockEntity(unique_id=f"unique{number}")
entity.entity_id = async_generate_entity_id(DOMAIN + ".{}", "Number", hass=hass)
return entity

Expand Down
30 changes: 0 additions & 30 deletions tests/ignore_uncaught_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
("tests.components.cast.test_media_player", "test_entry_setup_single_config"),
("tests.components.cast.test_media_player", "test_entry_setup_list_config"),
("tests.components.cast.test_media_player", "test_entry_setup_platform_not_ready"),
("tests.components.config.test_automation", "test_delete_automation"),
("tests.components.config.test_group", "test_update_device_config"),
("tests.components.default_config.test_init", "test_setup"),
("tests.components.demo.test_init", "test_setting_up_demo"),
Expand Down Expand Up @@ -46,20 +45,9 @@
("tests.components.dyson.test_fan", "test_purecool_update_state_filter_inv"),
("tests.components.dyson.test_fan", "test_purecool_component_setup_only_once"),
("tests.components.dyson.test_sensor", "test_purecool_component_setup_only_once"),
("test_homeassistant_bridge", "test_homeassistant_bridge_fan_setup"),
("tests.components.ios.test_init", "test_creating_entry_sets_up_sensor"),
("tests.components.ios.test_init", "test_not_configuring_ios_not_creates_entry"),
("tests.components.local_file.test_camera", "test_file_not_readable"),
("tests.components.meteo_france.test_config_flow", "test_user"),
("tests.components.meteo_france.test_config_flow", "test_import"),
("tests.components.mikrotik.test_device_tracker", "test_restoring_devices"),
("tests.components.mikrotik.test_hub", "test_arp_ping"),
("tests.components.mqtt.test_alarm_control_panel", "test_unique_id"),
("tests.components.mqtt.test_binary_sensor", "test_unique_id"),
("tests.components.mqtt.test_camera", "test_unique_id"),
("tests.components.mqtt.test_climate", "test_unique_id"),
("tests.components.mqtt.test_cover", "test_unique_id"),
("tests.components.mqtt.test_fan", "test_unique_id"),
(
"tests.components.mqtt.test_init",
"test_setup_uses_certificate_on_certificate_set_to_auto",
Expand All @@ -80,22 +68,14 @@
"tests.components.mqtt.test_init",
"test_setup_with_tls_config_of_v1_under_python36_only_uses_v1",
),
("tests.components.mqtt.test_legacy_vacuum", "test_unique_id"),
("tests.components.mqtt.test_light", "test_unique_id"),
("tests.components.mqtt.test_light", "test_entity_device_info_remove"),
("tests.components.mqtt.test_light_json", "test_unique_id"),
("tests.components.mqtt.test_light_json", "test_entity_device_info_remove"),
("tests.components.mqtt.test_light_template", "test_entity_device_info_remove"),
("tests.components.mqtt.test_lock", "test_unique_id"),
("tests.components.mqtt.test_sensor", "test_unique_id"),
("tests.components.mqtt.test_state_vacuum", "test_unique_id"),
("tests.components.mqtt.test_switch", "test_unique_id"),
("tests.components.mqtt.test_switch", "test_entity_device_info_remove"),
("tests.components.qwikswitch.test_init", "test_binary_sensor_device"),
("tests.components.qwikswitch.test_init", "test_sensor_device"),
("tests.components.rflink.test_init", "test_send_command_invalid_arguments"),
("tests.components.samsungtv.test_media_player", "test_update_connection_failure"),
("tests.components.tplink.test_init", "test_configuring_device_types"),
(
"tests.components.tplink.test_init",
"test_configuring_devices_from_multiple_sources",
Expand All @@ -108,18 +88,8 @@
("tests.components.unifi_direct.test_device_tracker", "test_get_scanner"),
("tests.components.upnp.test_init", "test_async_setup_entry_default"),
("tests.components.upnp.test_init", "test_async_setup_entry_port_mapping"),
("tests.components.vera.test_init", "test_init"),
("tests.components.wunderground.test_sensor", "test_fails_because_of_unique_id"),
("tests.components.yr.test_sensor", "test_default_setup"),
("tests.components.yr.test_sensor", "test_custom_setup"),
("tests.components.yr.test_sensor", "test_forecast_setup"),
("tests.components.zwave.test_init", "test_power_schemes"),
(
"tests.helpers.test_entity_platform",
"test_adding_entities_with_generator_and_thread_callback",
),
(
"tests.helpers.test_entity_platform",
"test_not_adding_duplicate_entities_with_unique_id",
),
]