Handle aiodns pycares exceptions "leaking"#170048
Conversation
home-assistant
Bot
added
bugfix
cla-signed
integration: dnsip
small-pr
There was a problem hiding this comment.
Pull request overview
This PR updates the dnsip integration to prevent unexpected pycares exceptions (raised via aiodns) from bubbling up and crashing hostname resolution / validation.
Changes:
- Broadened exception handling in the DNS IP sensor update loop to catch non-
DNSErrorfailures. - Broadened exception suppression in the config flow hostname validation checks to tolerate additional resolver exceptions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
homeassistant/components/dnsip/sensor.py |
Catch broader resolver exceptions during periodic DNS queries to avoid update crashes. |
homeassistant/components/dnsip/config_flow.py |
Suppress broader resolver exceptions during config-flow validation to avoid flow failures from pycares leaks. |
| except Exception as err: # noqa: BLE001 | ||
| # Handle leaking through other (pycares) exceptions than DNSError | ||
| _LOGGER.warning("Exception while resolving host: %s", err) | ||
| await self.resolver.close() |
| _LOGGER.debug("Timeout while resolving host: %s", err) | ||
| await self.resolver.close() | ||
| except DNSError as err: | ||
| except (aiodns.error.DNSError, AresError, asyncio.CancelledError) as err: |
There was a problem hiding this comment.
That's completely opposite of what we want
| tasks = await asyncio.gather( | ||
| async_check(hostname, resolver_ipv4, "A", port=port), | ||
| async_check(hostname, resolver_ipv6, "AAAA", port=port_ipv6), | ||
| async_check(hostname, resolver_ipv4, "AAAA", port=port), | ||
| return_exceptions=True, | ||
| ) | ||
|
|
||
| result[CONF_IPV4] = tasks[0] | ||
| result[CONF_IPV6] = tasks[1] | ||
| result[CONF_IPV6_V4] = tasks[2] | ||
| result[CONF_IPV4] = bool(tasks[0]) if not isinstance(tasks[0], Exception) else False | ||
| result[CONF_IPV6] = bool(tasks[1]) if not isinstance(tasks[1], Exception) else False | ||
| result[CONF_IPV6_V4] = ( | ||
| bool(tasks[2]) if not isinstance(tasks[2], Exception) else False | ||
| ) |
| async with asyncio.timeout(10): | ||
| if self.resolver._closed: # noqa: SLF001 | ||
| self.create_dns_resolver() | ||
| response = await self.resolver.query(self.hostname, self.querytype) |
There was a problem hiding this comment.
Is for a separate PR
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
A few issues that should be addressed before merging.
The main concern is around asyncio.CancelledError handling: catching it without re-raising will swallow task cancellation and can prevent Home Assistant from shutting down or reloading cleanly. There is also a subtle type hierarchy issue with the isinstance check in the config flow on Python 3.14.
| await self.resolver.close() | ||
| if self.resolver: | ||
| await self.resolver.close() | ||
| except (aiodns.error.DNSError, AresError, asyncio.CancelledError) as err: |
There was a problem hiding this comment.
Catching asyncio.CancelledError without re-raising it will swallow task cancellation. When Home Assistant is shutting down or reloading, the update task may be cancelled, and if CancelledError is caught and silenced here, the shutdown/reload can hang (until the 10 second timeout fires).
Suggestion: handle CancelledError separately and re-raise it after cleanup, or remove it from this except clause entirely:
except asyncio.CancelledError:
if self.resolver:
await self.resolver.close()
raise
except (aiodns.error.DNSError, AresError) as err:
_LOGGER.debug("Exception while resolving host: %s", err)
if self.resolver:
await self.resolver.close()| result[CONF_IPV4] = tasks[0] | ||
| result[CONF_IPV6] = tasks[1] | ||
| result[CONF_IPV6_V4] = tasks[2] | ||
| result[CONF_IPV4] = bool(tasks[0]) if not isinstance(tasks[0], Exception) else False |
There was a problem hiding this comment.
In Python 3.14, asyncio.CancelledError inherits from BaseException, not Exception. With return_exceptions=True, if a task gets cancelled, the CancelledError instance is returned as a result. Then isinstance(tasks[0], Exception) returns False, and bool(CancelledError()) evaluates to True, making it look like DNS resolution succeeded when it was actually cancelled.
Consider using BaseException instead:
result[CONF_IPV4] = bool(tasks[0]) if not isinstance(tasks[0], BaseException) else FalseOr check for the expected success type directly:
result[CONF_IPV4] = isinstance(tasks[0], list) and bool(tasks[0])| if self.resolver: | ||
| await self.resolver.close() | ||
| except (aiodns.error.DNSError, AresError, asyncio.CancelledError) as err: | ||
| _LOGGER.debug("Exception while resolving host: %s", err) |
There was a problem hiding this comment.
This changed from _LOGGER.warning to _LOGGER.debug. Legitimate persistent DNS errors will no longer be visible to users at the default log level. Was this intentional? Since the entity will go unavailable after retries are exhausted that might be sufficient as a signal, but worth confirming.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
changelog: aio-libs/aiodns@v4.0.0...v4.0.2 replaces and closes home-assistant#170048
Breaking change
Proposed change
Background: aio-libs/aiodns#231
pycaresexceptions leaks through and raises so until addressed we need to catchExceptionType of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: