Skip to content

Handle aiodns pycares exceptions "leaking" #170048

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

Closed
gjohansson-ST wants to merge 8 commits into from
Closed

Handle aiodns pycares exceptions "leaking" #170048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2969d62
Handle aiodns pycares exceptions "leaking"
gjohansson-ST May 7, 2026
ddc8343
Fix
gjohansson-ST May 10, 2026
733937f
close resolver
gjohansson-ST May 10, 2026
fd6a423
Add to test
gjohansson-ST May 10, 2026
5b1ad7d
fix
gjohansson-ST May 10, 2026
79d6084
Fix
gjohansson-ST May 10, 2026
3a81bfb
Move to debug
gjohansson-ST May 10, 2026
6a367d3
type
gjohansson-ST May 10, 2026
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
28 changes: 15 additions & 13 deletions homeassistant/components/dnsip/config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
"""Adds config flow for dnsip integration."""

import asyncio
import contextlib
from typing import Any, Literal

import aiodns
from aiodns.error import DNSError
import voluptuous as vol

from homeassistant.config_entries import (
Expand Down Expand Up @@ -61,15 +59,16 @@ async def async_validate_hostname(

async def async_check(
hostname: str, resolver: str, qtype: Literal["A", "AAAA"], port: int = 53
) -> bool:
"""Return if able to resolve hostname."""
result: bool = False
with contextlib.suppress(DNSError):
_resolver = aiodns.DNSResolver(
nameservers=[resolver], udp_port=port, tcp_port=port
)
result = bool(await _resolver.query(hostname, qtype))
) -> list:
"""Return list of hostnames."""
Comment thread
gjohansson-ST marked this conversation as resolved.

_resolver = aiodns.DNSResolver(
nameservers=[resolver], udp_port=port, tcp_port=port
)
try:
result = await _resolver.query(hostname, qtype)
finally:
await _resolver.close()
return result

result: dict[str, bool] = {}
Expand All @@ -78,11 +77,14 @@ async def async_check(
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
Copy link
Copy Markdown
Member

@frenck frenck May 15, 2026

Choose a reason for hiding this comment

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

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 False

Or check for the expected success type directly:

result[CONF_IPV4] = isinstance(tasks[0], list) and bool(tasks[0])

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
)

return result

Expand Down
16 changes: 9 additions & 7 deletions homeassistant/components/dnsip/sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from typing import Literal

import aiodns
from aiodns.error import DNSError
from pycares import AresError

Comment thread
gjohansson-ST marked this conversation as resolved.
from homeassistant.components.sensor import SensorEntity
from homeassistant.config_entries import ConfigEntry
Expand Down Expand Up @@ -114,18 +114,20 @@ def create_dns_resolver(self) -> None:

async def async_update(self) -> None:
"""Get the current DNS IP address for hostname."""
if self.resolver._closed: # noqa: SLF001
self.create_dns_resolver()
response = None
try:
async with asyncio.timeout(10):
if self.resolver._closed: # noqa: SLF001
self.create_dns_resolver()
response = await self.resolver.query(self.hostname, self.querytype)
Comment on lines 119 to 122
Copy link
Copy Markdown
Member Author

@gjohansson-ST gjohansson-ST May 10, 2026

Choose a reason for hiding this comment

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

Is for a separate PR

except TimeoutError as err:
_LOGGER.debug("Timeout while resolving host: %s", err)
await self.resolver.close()
except DNSError as err:
_LOGGER.warning("Exception while resolving host: %s", err)
await self.resolver.close()
if self.resolver:
await self.resolver.close()
except (aiodns.error.DNSError, AresError, asyncio.CancelledError) as err:
Copy link
Copy Markdown
Member Author

@gjohansson-ST gjohansson-ST May 10, 2026

Choose a reason for hiding this comment

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

That's completely opposite of what we want

Comment thread
gjohansson-ST marked this conversation as resolved.
Copy link
Copy Markdown
Member

@frenck frenck May 15, 2026

Choose a reason for hiding this comment

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

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

_LOGGER.debug("Exception while resolving host: %s", err)
Copy link
Copy Markdown
Member

@frenck frenck May 15, 2026

Choose a reason for hiding this comment

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

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.

if self.resolver:
await self.resolver.close()

if response:
sorted_ips = sort_ips(
Expand Down
12 changes: 10 additions & 2 deletions tests/components/dnsip/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from unittest.mock import patch

from aiodns.error import DNSError
from pycares import AresError
import pytest

from homeassistant import config_entries
Expand Down Expand Up @@ -121,15 +122,22 @@ async def test_form_adv(hass: HomeAssistant) -> None:
assert len(mock_setup_entry.mock_calls) == 1


async def test_form_error(hass: HomeAssistant) -> None:
@pytest.mark.parametrize(
"error",
[
(DNSError),
(AresError),
],
)
async def test_form_error(hass: HomeAssistant, error: type[Exception]) -> None:
"""Test validate url fails."""
result = await hass.config_entries.flow.async_init(
DOMAIN, context={"source": config_entries.SOURCE_USER}
)

with patch(
"homeassistant.components.dnsip.config_flow.aiodns.DNSResolver",
side_effect=DNSError("Did not find"),
side_effect=error("Did not find"),
):
result2 = await hass.config_entries.flow.async_configure(
result["flow_id"],
Expand Down