feat(mdns): only send listening addresses that match interface#6003
feat(mdns): only send listening addresses that match interface#6003elenaf9 wants to merge 8 commits intolibp2p:masterfrom
Conversation
protocols/mdns/src/behaviour.rs
Outdated
| 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. |
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