Skip to content

Improve Matter DNS-SD service parsing #1144

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

Merged
merged 4 commits into from
May 8, 2025
Merged

Conversation

agners
Copy link
Collaborator

@agners agners commented May 7, 2025

Services are now parsed in a more robust way, to avoid issues in cases where the service name is not exactly as expected. This has been observed to be the case for Thread devices, presumably because two Thread border routers attempt to announce the same service name. The service name then becomes a " (2)" suffix, which the old code did not handle well.

We could parse this duplicated messages as well, but generally only a single node should ever announce a service name. If a service name gets announced by two independent devices, the two services likely stem from the same underlying device but simply got duplicated by some environmental factor (e.g. Thread border routers not being in sync on who announces the service name, or badly configured mDNS reflectors duplicating messages on the same network). Only processing the correctly formatted service name is sufficient in these cases.

@agners
Copy link
Collaborator Author

agners commented May 7, 2025

This prevents errors like this:

2025-04-24 13:46:58.616 (MainThread) ERROR [matter_server.server] Error doing task: Exception in callback MatterDeviceController._on_mdns_operational_node_state('A3604AC231F8...r._tcp.local.', <ServiceState...ge.Updated: 3>)
Traceback (most recent call last):
  File "/usr/local/lib/python3.12/asyncio/events.py", line 88, in _run
    self._context.run(self._callback, *self._args)
  File "/usr/local/lib/python3.12/site-packages/matter_server/server/device_controller.py", line 1544, in _on_mdns_operational_node_state
    node_id = int(name.split("-")[1].split(".")[0], 16)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 16: '00000000000000AD (6)'

Copy link
Collaborator

@marcelveldt marcelveldt left a comment

Choose a reason for hiding this comment

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

LGTM!

BTW: At some point we should write a simple test to test a bunch of dns names against this regex

@agners agners added maintenance Code (quality) improvement or small enhancement which not a new feature bugfix Pull request that fixes a (known) issue/bug labels May 7, 2025
agners and others added 3 commits May 8, 2025 09:27
Services are now parsed in a more robust way, to avoid issues in cases
where the service name is not exactly as expected. This has been
observed to be the case for Thread devices, presumably because two
Thread border routers attempt to announce the same service name.
The service name then becomes a " (2)" suffix, which the old code
did not handle well.

We could parse this duplicated messages as well, but generally only
a single node should ever announce a service name. If a service name
gets announced by two independent devices, the two services likely
stem from the same underlying device but simply got duplicated by
some environmental factor (e.g. Thread border routers not being in
sync on who announces the service name, or badly configured mDNS
reflectors duplicating messages on the same network). Only processing
the correctly formatted service name is sufficient in these cases.
@agners agners force-pushed the improve-dns-sd-service-parsing branch from a288836 to 7499a7b Compare May 8, 2025 07:27
Instead of relying on pre-commit to pass all filenames, call pylint
explicitly on the directories we want to check. This avoids that
pylint potentially adds directories to the Python path, leading to
unexpected imports (e.g. Module 'json' has no 'load' member, when
pylint gets called with `pylint matter_server/server/ota/__init__.py matter_server/common/helpers/util.py`).
@agners agners force-pushed the improve-dns-sd-service-parsing branch from 1581483 to 3a6489b Compare May 8, 2025 08:50
@agners agners merged commit 747a320 into main May 8, 2025
4 checks passed
@agners agners deleted the improve-dns-sd-service-parsing branch May 8, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a (known) issue/bug maintenance Code (quality) improvement or small enhancement which not a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants