Conversation
| if let Some(tx) = self.if_tasks.get_mut(&ip) { | ||
| if tx.unbounded_send(update).is_err() { | ||
| tracing::error!("`InterfaceState` for ip {ip} dropped"); | ||
| self.if_tasks.remove(&ip); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ideally this sender should be bounded. I still have to look into how to solve this best, I am currently unsure how to handle the case that a interface-task is busy and the channel full.
Requires us to handle the case that the channel is full. Solved by adding a `pending_address_update` queue that is processed in `Behavior::poll`.
|
Any chance to continue with this fix? This is currently keeping me from using upstream libp2p and means I have to use my awkwardly patched fork |
|
This pull request has merge conflicts. Could you please resolve them @elenaf9? 🙏 |
…eat/mdns/only-send-if-addresses
…eat/mdns/only-send-if-addresses
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
thanks for the simplification and addressing the issue Elena, Looks good overall.
Left a minor comment wrt MSRV
| local_peer_id, | ||
| pending_events: Default::default(), | ||
| pending_address_updates: Default::default(), | ||
| waker: Waker::noop().clone(), |
There was a problem hiding this comment.
std::task::Waker::noop was only stabilized in 1.85 our current MSRV is 1.83.
| } | ||
| if let Entry::Vacant(e) = self.if_tasks.entry(addr) { | ||
| if let Entry::Vacant(e) = self.if_tasks.entry(ip_addr) { | ||
| let (addr_tx, addr_rx) = mpsc::channel(10); // Chosen arbitrarily. |
There was a problem hiding this comment.
curiosity Elena, why did you prefer an "unbounded" Vec for pending_address_updates vs unbounded channels for the listen_addresses_rx
|
I'm still waiting for this. Any chance to make progress here? |
|
So, since I've been waiting for months on this now, I set copilot to it. It came up with a much simpler solution, though I'm not sure if it's complete. You can check it out at MatthiasvB@c831c49 It also removed the crude hotfix that filtered loopback addresses on that branch, so ignore that. If you think that fix is proper, feel free to integrate it here as well. If you accept "AI contributions", that is. |
…IP (#6500) When responding to mDNS queries, only advertise listening addresses whose IP matches the address family / interface the query arrived on, instead of advertising every listening address. Avoids announcing unreachable addresses. ## Description mDNS currently wildly reports all interface's addresses on all interfaces, which leads to mutiple issues: Fixes #5995. Fixes #5790. There already is a PR, but it's been open for a long time and seems more complex: #6003 ## AI Assistance Disclosure **Tools used** Github Copilot (initially), rework using Claude Code (Opus 4.8) **Attestation** _(required)_: - [x] I have read every line of this diff, understand what it does, and can explain it in review. ## Notes & open questions The fix seems smart and in the right place, but I lack complete overview over the crate, so some expert review should verify this is a good spot for the fix ## Change checklist - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation (not applicable) - [x] I have added tests that prove my fix is effective or that my feature works - [x] A changelog entry has been made in the appropriate crates Approved-by: elenaf9 <57632201+elenaf9@users.noreply.github.com>
|
Superseded by #6500. |
3 participants
Description
The current mDNS implementation always sends all current listen addresses in a
MdnsResponse, independent of whether they match the interface that we receive aMdnsQueryon.This is causing multiple issues, e.g.:
And as noted in #5790, it also violates the mDNS specs.
This PR changes
libp2p-mdnsto only reply to aMdnsQuerywith the listen addresses that match the IP of the interface that we received the query on.Implementation-wise, this is solved by providing each interface-task only with the subset of listening addresses that match the interface IP.
Fixes: #5995
Related: #5790
Notes & open questions
Because each interface-task now has a different list of listening addresses that it will send out, I think it doesn't make sense anymore to have a single shared list of listening addresses. Instead, I've changed the implementation to only provide each interface-task with the listening addresses that match the interface's IP, and use a channel for informing the task about relevant listening address updates.
cc @MathJud @T-X @MatthiasvB
Change checklist