feat: add multicast_addresses to bind/multicast independently - #1767
feat: add multicast_addresses to bind/multicast independently#1767bluetoothbot wants to merge 6 commits into
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #1767 +/- ##
=======================================
Coverage 99.77% 99.77%
=======================================
Files 33 33
Lines 3536 3562 +26
Branches 498 508 +10
=======================================
+ Hits 3528 3554 +26
Misses 5 5
Partials 3 3 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
Merging this PR will not alter performance✅ 14 untouched benchmarks Comparing bluetoothbot:koan/fix-issue-1520 (6e25708) with master (cb81e67) |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds a multicast_addresses escape hatch to let callers join additional multicast groups on the listen socket without also binding per-interface respond sockets—targeting sandboxed environments (e.g. iOS) where binding :5353 on physical interfaces can be blocked while multicast membership is still allowed/needed.
Changes:
- Extend create_sockets() with multicast_addresses and reject multicast_addresses with unicast=True.
- Thread the new parameter through Zeroconf and AsyncZeroconf constructors (docs + typing included).
- Add unit tests for IPv4/IPv6 membership behavior, the InterfaceChoice.Default fast path, unicast rejection, and Zeroconf-level wiring.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file| File | Description |
|---|---|
| src/zeroconf/_utils/net.py | Adds multicast_addresses handling in create_sockets() to join extra multicast memberships on the listen socket. |
| src/zeroconf/_core.py | Exposes/forwards multicast_addresses from Zeroconf.__init__() into socket creation and documents behavior. |
| src/zeroconf/asyncio.py | Exposes/forwards multicast_addresses from AsyncZeroconf.__init__() into Zeroconf. |
| tests/utils/test_net.py | Adds focused socket-creation tests covering IPv4/IPv6, default fast path, and unicast incompatibility. |
| tests/test_core.py | Adds a wiring test that verifies multicast_addresses is forwarded to create_sockets(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| listen_socket, respond_sockets = create_sockets( | ||
| interfaces, | ||
| unicast, | ||
| ip_version, | ||
| apple_p2p=apple_p2p, | ||
| multicast_addresses=multicast_addresses, | ||
| ) |
| if not unicast and interfaces is InterfaceChoice.Default and ip_version != IPVersion.All: | ||
| for interface in normalized_interfaces: | ||
| add_multicast_member(cast(socket.socket, listen_socket), interface) | ||
| for interface in extra_multicast_members: | ||
| add_multicast_member(cast(socket.socket, listen_socket), interface) |
| zc = r.Zeroconf( | ||
| interfaces=["127.0.0.1"], | ||
| multicast_addresses=["192.168.1.5"], | ||
| unicast=True, |
| normalized_interfaces = normalize_interface_choice(interfaces, ip_version) | ||
| extra_multicast_members = ( | ||
| normalize_interface_choice(list(multicast_addresses), ip_version) if multicast_addresses else [] | ||
| ) |
PR Review — feat: add multicast_addresses to bind/multicast independentlyThe feature itself is a clean, additive escape hatch with sensible behaviour: extras are validated up-front against ip_version, deduped against the existing interfaces set, and joined on the listen socket in both the Default fast path and the per-interface path. The unicast=True rejection is correct (there's no listen socket to join groups on). Three things to address before merging: (1) tuple entries in multicast_addresses are accepted by the type signature and the validator but silently dropped by normalize_interface_choice — this needs to either be supported or explicitly removed from the type; (2) @bdraco's DRY request — pull the validation, dedup, and extras-join into small helpers so create_sockets stays a coordinator; (3) the TC-deferral test rewrite is unrelated scope and should ship in its own PR. The test-coverage gap @bdraco flagged earlier appears to have been fixed per the latest Codecov run. No correctness or security concerns in the wire-protocol path. 🟡 Important1. Tuple entries silently dropped — type signature claims support, runtime ignores them (`src/zeroconf/_utils/net.py`, L469-475)multicast_addresses is typed as Sequence[str | int | tuple[tuple[str, int, int], int]], but passing a pre-normalized IPv6 tuple silently produces no join. Follow the chain:
Net effect: multicast_addresses=[(('fe80::1', 0, 0), 1)] looks legitimate per the annotation, passes validation, and adds zero multicast memberships. There is no error, no log line — just a feature that silently doesn't work for one of its three documented input types. Two options: either (a) extend ip6_addresses_to_indexes to pass tuple entries through (elif isinstance(iface, tuple): result.append(iface)), keeping the type signature honest; or (b) tighten the parameter type to Sequence[str | int] and document that callers must supply addresses/indexes, not pre-normalized tuples. Whichever you pick, add a test that exercises the chosen contract. if multicast_addresses:
extra_multicast_members = normalize_interface_choice(list(multicast_addresses), ip_version)
# Strip entries already covered by ``interfaces`` so add_multicast_member
# is not called twice for the same membership.
interface_set = set(normalized_interfaces)
extra_multicast_members = [m for m in extra_multicast_members if m not in interface_set]
The diff to test_tc_bit_defers_last_response_missing (widening _TC_DELAY_RANDOM_INTERVAL to (1500, 60_000) and bumping the post-test cleanup loop from 8 iterations to 30) has nothing to do with the multicast_addresses feature this PR introduces. It looks like a separate flaky-test fix that got bundled in. This matters because:
Please split this hunk out into its own follow-up PR with a test: or ci: prefix and an explanation of the flakiness it's chasing. Keep this PR focused on multicast_addresses and create_sockets/autodetect_ip_version wiring. # Widen the per-packet delay so Windows scheduling jitter between
# assertions cannot fire the timer mid-test, while leaving plenty of
# headroom under the first-arrival deadline for timer-replacement to
# remain deterministic.
with patch.object(_listener, "_TC_DELAY_RANDOM_INTERVAL", (1500, 60_000)):
🟢 Suggestions1. Extract helpers to address @bdraco's DRY request (`src/zeroconf/_utils/net.py`, L455-520)@bdraco asked for this to be made more DRY. The for interface in extra_multicast_members: add_multicast_member(listen_socket, interface) loop appears verbatim on both the Default fast path and the per-interface path. The IP-version validation block at the top of create_sockets is also a discrete concern that's worth its own helper. Consider: def _validate_multicast_addresses(
multicast_addresses: Sequence[...] | None, ip_version: IPVersion, unicast: bool,
) -> None:
if not multicast_addresses:
return
if unicast:
raise ValueError("multicast_addresses is incompatible with unicast=True")
if ip_version == IPVersion.V4Only and any(_entry_ip_version(e) == 6 for e in multicast_addresses):
raise ValueError("multicast_addresses contains IPv6 entries but ip_version is V4Only")
if ip_version == IPVersion.V6Only and any(_entry_ip_version(e) == 4 for e in multicast_addresses):
raise ValueError("multicast_addresses contains IPv4 entries but ip_version is V6Only")
def _resolve_extra_multicast_members(
multicast_addresses: Sequence[...] | None,
normalized_interfaces: list[...],
ip_version: IPVersion,
) -> list[...]:
if not multicast_addresses:
return []
extras = normalize_interface_choice(list(multicast_addresses), ip_version)
interface_set = set(normalized_interfaces)
return [m for m in extras if m not in interface_set]
def _join_extra_multicast_members(listen_socket, extras):
for member in extras:
add_multicast_member(listen_socket, member)That collapses create_sockets back to roughly its original shape with three named hooks for the new behaviour, and lets each helper be tested directly. Checklist
SummaryThe feature itself is a clean, additive escape hatch with sensible behaviour: extras are validated up-front against ip_version, deduped against the existing interfaces set, and joined on the listen socket in both the Default fast path and the per-interface path. The unicast=True rejection is correct (there's no listen socket to join groups on). Three things to address before merging: (1) tuple entries in multicast_addresses are accepted by the type signature and the validator but silently dropped by normalize_interface_choice — this needs to either be supported or explicitly removed from the type; (2) @bdraco's DRY request — pull the validation, dedup, and extras-join into small helpers so create_sockets stays a coordinator; (3) the TC-deferral test rewrite is unrelated scope and should ship in its own PR. The test-coverage gap @bdraco flagged earlier appears to have been fixed per the latest Codecov run. No correctness or security concerns in the wire-protocol path. To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all. Automated review by Kōanf265d95 |
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Changes applied
Stats5 files changed, 237 insertions(+), 10 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
bdraco
left a comment
There was a problem hiding this comment.
Need to complete coverage
src/zeroconf/_utils/net.py 96.66% 0 Missing and 1 partia
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Changes applied
Stats5 files changed, 273 insertions(+), 10 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Stats5 files changed, 273 insertions(+), 10 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
There was a problem hiding this comment.
Seems like we already have similar logic and this could be made more DRY by breaking out into helpers
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Changes applied
Stats5 files changed, 274 insertions(+), 15 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
bdraco
left a comment
There was a problem hiding this comment.
coverage is incomplete
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/zeroconf/_utils/net.py | 93.54% | 1 Missing and 1 partial ⚠️ |
Files with missing lines Patch % Lines
src/zeroconf/_utils/net.py 93.54% 1 Missing and 1 partial ⚠️
Sorry, something went wrong.
Sorry, something went wrong.
iOS and other sandboxed platforms allow joining the mDNS multicast group on a physical interface but forbid binding port 5353 on it because the system mDNS daemon owns the port. The existing ``interfaces`` parameter conflates the two: every entry both joins the multicast group and gets a respond socket bound to that interface's address. Add an optional ``multicast_addresses`` parameter on ``Zeroconf`` / ``AsyncZeroconf`` / ``create_sockets`` for additional addresses that are joined to the listen socket's multicast group without creating a respond socket. The default behavior is unchanged. Fixes python-zeroconf#1520
…up against interfaces
… validation and autodetect
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Changes applied
Stats5 files changed, 319 insertions(+), 52 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
| normalized_interfaces = normalize_interface_choice(interfaces, ip_version) | ||
| if multicast_addresses: | ||
| extra_multicast_members = normalize_interface_choice(list(multicast_addresses), ip_version) | ||
| # Strip entries already covered by ``interfaces`` so add_multicast_member | ||
| # is not called twice for the same membership. |
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-issue-1520 was rebased onto master and review feedback was applied. Changes applied
Stats5 files changed, 347 insertions(+), 52 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
Summary
Adds a new multicast_addresses parameter to Zeroconf / AsyncZeroconf /
create_sockets so multicast group membership can be configured
independently of bind interfaces. On iOS (and other sandboxed
environments) the system mDNS daemon owns port 5353 on physical
interfaces so binding there is rejected, but joining the multicast
group on those interfaces is still permitted and necessary to receive
incoming queries.
Today the interfaces parameter conflates the two roles: every
entry both joins the multicast group and gets a respond socket
bound on it. multicast_addresses adds an additive escape hatch
that joins additional addresses to the listen socket's multicast
group without creating respond sockets for them. Default behavior is
unchanged.
Fixes #1520
Changes
normalized address to the listen socket; rejects the combination
with unicast=True since there is no listen socket.
parameter and forward it through.
path and the per-interface respond-socket path.
rejection, and Zeroconf-level wiring.
Test plan
files
Quality Report
Changes: 5 files changed, 148 insertions(+), 3 deletions(-)
Code scan: clean
Tests: passed (4 PASSED)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline