Replace asyncio.wait with asyncio.gather since wait ignores exceptions by ziv1234 · Pull Request #33380 · home-assistant/core

ziv1234 · 2020-03-28T23:19:33Z

fix for test_entity_platform so it expects the exception

Breaking change

Not a breaking change

Proposed change

Fix for the helpers/test_entity_platform exceptions
Replaced asyncio.wait with asyncio.gather since wait doesn't propagate the exceptions to the waiting function. Once there, a duplicate unique_id triggers an exception that the test now catches.
Not my code, but just a suggestion

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

balloob · 2020-03-29T03:53:30Z


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


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.

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

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

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?

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 :)

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

We should log it in this method where we change from wait to gather. Gather can return exceptions.

Yeah that's bad. I'll add one.

Yeah

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?

ziv1234 · 2020-03-30T12:25:37Z

also, the same issue exists with async_add_entities with duplicate IDs. an exception gets thrown and never gets caught. This is the thing that fails the mqtt tests.
again, you guys know what the right behavior for the system, but if we don't want to propagate the exception, probably better to replace the exception with an error log.
Let me know what behavior is best and i can make the change.

balloob · 2020-03-30T23:02:48Z

Was actually just looking at this to add the tests (and got distracted obviously), but here is a quick update:

EntityPlatform should not raise HomeAssistantError on malformed or duplicate entity ID but instead should print an error and ignore entity.

ziv1234 · 2020-03-31T00:04:04Z

Was actually just looking at this to add the tests (and got distracted obviously), but here is a quick update:

EntityPlatform should not raise HomeAssistantError on malformed or duplicate entity ID but instead should print an error and ignore entity.

I agree. Will make the change tomorrow

ziv1234 · 2020-03-31T07:44:04Z

ok. made the change in entity_platform so it doesn't throw exception on duplicate unique_id and simply logs and returns.
I edited the list of ignored_exceptions, but the problem is that there are several tests that fail in the CI and succeed locally. Do you know why this can happen?
Hopefully I finally got the list right, but concerned about tests passing locally and not the CI as I try to not push a commit that fails tests...

fix for test_entity_platform so it expects the exception

…he CI

…with log and return reverted test_entity_platform to its original since now there is no exception thrown

…lly fail in the CI

balloob · 2020-03-31T17:36:49Z

+                )
+            else:
+                _LOGGER.error(
+                    "Error setting up integration %s - returned False", domain


The entity platform is already dealing with return value of False

ok, so just remove the log in that case?

balloob · 2020-03-31T22:55:09Z

+        await asyncio.wait(futures.values())
+        errors = [domain for domain in domains if futures[domain].exception()]
+        for domain in errors:
+            _LOGGER.error(


For this one, we have set up async_setup_component in such a way that we already wrap every piece of code that we call from an integration with try…catch. So if any of this raises, it's truly unexpected. So we should make sure we pass the exception as exc_info so we can print a stack trace

balloob · 2020-03-31T23:36:23Z

+                "Error setting up integration %s - received exception",
                domain,
-                futures[domain].exception(),
+                **kwargs,  # type: ignore


Why do it like this and not do exc_info=(type(exception), exception, exception.__traceback__) ?

i just copied it from the default exception handler in core.py, but you are correct. i made the change. do you want me to change the one in core.py as well?

BTW, if i understand the python behavior correctly, the one in core.py can also be replaced by exc_info=True, as it happens during the context of the exception, but not sure as i am new to python

Yes, if we're within a catch block, we should let the logger get it themselves.

ziv1234 · 2020-04-01T16:08:20Z

thanks! one thing that is still puzzling me is that there are some tests (at least some of dyson and the localfile test) that fail on the CI if i remove them from the ignore list and they run fine locally, both with pytest and tox.
Now that this has been merged, i can recreate in a separate "dummy" PR and open an issue, since it would be great if the local tests would simulate the CI as well as possible

balloob · 2020-04-01T16:55:11Z

On CI we run things in parallel with -n auto and based on the number of cores, it means the order of tests can be different, which might impact it.

ziv1234 · 2020-04-01T18:05:43Z

Ok. I find it failing quite predictably in the CI on specific tests and i wasn't able to get them to fail locally. I opened an issue (#33504) to try and understand because it happens consistently.

Anything else that is different? In general, the tests are usually running independent of each other as each one creates a new hass fixture, right?

MartinHjelmare · 2020-04-01T18:39:18Z

CI VMs can be slow so time difference can be greater which can matter when playing with time, eg not patching time correctly and firing time changed events.

homeassistant added the cla-signed label Mar 28, 2020

probot-home-assistant Bot added core small-pr PRs with less than 30 lines. labels Mar 28, 2020

ziv1234 force-pushed the fix_2 branch from 40d11ba to 7e23b64 Compare March 29, 2020 00:03

balloob reviewed Mar 29, 2020

View reviewed changes

MartinHjelmare changed the title ~~replace asyncio.wait with asyncio.gather since wait ignores exceptions~~ Replace asyncio.wait with asyncio.gather since wait ignores exceptions Mar 29, 2020

ziv1234 force-pushed the fix_2 branch from 7d42a8f to ef9ac1c Compare March 30, 2020 22:50

ziv1234 force-pushed the fix_2 branch from 861f59b to 529196b Compare March 31, 2020 06:43

MartinHjelmare reviewed Mar 31, 2020

View reviewed changes

Comment thread homeassistant/bootstrap.py Outdated

MartinHjelmare reviewed Mar 31, 2020

View reviewed changes

Comment thread homeassistant/helpers/entity_platform.py

ziv1234 added 10 commits March 31, 2020 16:27

replace asyncio.wait with asyncio.gather since wait ignores exceptions

de40b61

fix for test_entity_platform so it expects the exception

changed to log on failed domains

f649a7a

discovered that this fix actually removes other uncaught exceptions

f48b612

fix in the list of ignored exceptions

265e616

replaced a few ignores on dyson tests that work locally but fail in t…

f64a6c4

…he CI

two more tests that are failing on the CI and not locally

a226537

removed assertion on multiple entries with same unique_id - replaced …

67f8738

…with log and return reverted test_entity_platform to its original since now there is no exception thrown

entered all the dyson tests. they all pass locally and probabilistica…

d253ea0

…lly fail in the CI

removed unnecessary str() for exception

c3ea753

added log message for duplicate entity_id / unique_id

fd3eab6

ziv1234 force-pushed the fix_2 branch from 65b417f to fd3eab6 Compare March 31, 2020 16:28

balloob reviewed Mar 31, 2020

View reviewed changes

removed log in case of False return value

323e21f

balloob reviewed Mar 31, 2020

View reviewed changes

added exc_info

9f62850

balloob reviewed Mar 31, 2020

View reviewed changes

change the use of exc_info

0934ac9

balloob approved these changes Apr 1, 2020

View reviewed changes

balloob merged commit 4dbbf93 into home-assistant:dev Apr 1, 2020

lock Bot locked and limited conversation to collaborators Apr 2, 2020

Uh oh!

Conversation

ziv1234 commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziv1234 Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziv1234 commented Mar 30, 2020

Uh oh!

balloob commented Mar 30, 2020

Uh oh!

ziv1234 commented Mar 31, 2020

Uh oh!

ziv1234 commented Mar 31, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziv1234 commented Apr 1, 2020

Uh oh!

balloob commented Apr 1, 2020

Uh oh!

ziv1234 commented Apr 1, 2020

Uh oh!

MartinHjelmare commented Apr 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ziv1234 commented Mar 28, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

ziv1234 Mar 29, 2020 •

edited

Loading