Skip to content
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

feat(mdns): emit ToSwarm::NewExternalAddrOfPeer #5179

Closed
wants to merge 2 commits into from
Closed

feat(mdns): emit ToSwarm::NewExternalAddrOfPeer #5179

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73e4fd6
ch
Feb 21, 2024
3f1893e
n
Feb 21, 2024
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
6 changes: 4 additions & 2 deletions protocols/mdns/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,10 @@ where
}

if !discovered.is_empty() {
let event = Event::Discovered(discovered);
return Poll::Ready(ToSwarm::GenerateEvent(event));
return Poll::Ready(ToSwarm::NewExternalAddrOfPeer {
Copy link
Contributor

@retrohacker retrohacker Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

[question]

Sorry if this is a silly question, I'm new to both rust and the libp2p code base.

  1. This seems like a breaking change for any code base that currently relies on Event::Discovered

Is my understanding of rust breaking changes right here?

Would it be worth preserving Event::Discovered here for backwards compatibility?

  1. This only emits the first discovered peer

Reading lines 333-345, it looks like the Poll event will keep going until it drains query_response_receiver, discovering multiple peers?

This seems like we would want to return a ToSwarm::NewExternalAddrOfPeer for every peer we discover on line 345.

Should this move up to line 345 and replace the discovered vec?

  1. I acknowledge that, as written, point 2 might be at odds with point 1 😅.

To have our cake, and eat it too, could 333-347 be moved out of fn poll and instead feed two receivers, one that sends Event::Discovered after all ToSwarm::NewExternalAddrOfPeers have been flushed and the other that sends each ToSwarm::NewExternalAddrOfPeer?

Copy link
Member

Choose a reason for hiding this comment

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

yeah exactly, thanks @retrohacker!
You can see the upnp example on how to return both

peer_id: discovered[0].0,
address: discovered[0].1.clone(),
});
}
// Emit expired event.
let now = Instant::now();
Expand Down
Loading