-
Notifications
You must be signed in to change notification settings - Fork 171
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
feat(iroh-net): local swarm discovery #2376
Conversation
9587dbe
to
56deadd
Compare
It would be great if there could be an automatic fallback to local discovery when the relay service is not reachable, rather than being forced to choose between the two. |
@zh522130 AFAIK this will be the case - you can enable/disable mdns and using a relay independently of each other! And when you enable both, it'll use whatever is available to try to establish a connection :) |
56deadd
to
8b4c0df
Compare
IIUC, there's a different way to do mDNS discovery. I'd say the two types are "swarm discovery" and "query based discovery". I think both approaches have ups and downs - so I'd prefer not to squat the mDNS name - at least not "just" as |
|
Very good, the previous version was reporting errors during testing, but I tested the latest code today and it is now functioning properly. |
} | ||
Ok(msg) => msg, | ||
}; | ||
match msg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be a single match statement
match msg {
Err(err) => {}
Ok(variant..) => {}
...
}
impl Discovery for LocalNodeDiscovery { | ||
fn resolve(&self, _ep: Endpoint, node_id: NodeId) -> Option<BoxStream<Result<DiscoveryItem>>> { | ||
let (send, recv) = flume::bounded(20); | ||
self.sender.send(Message::SendAddrs((node_id, send))).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses the blocking send
from flume, which can potentially cause blocks in the async context if this is called a lot and the channel gets full, no?
Edit: Can this deadlock even? If the channel is full and the receiving tokio task happens to be scheduled on the same thread, then sending into the full channel will block forever, because we never yield and the receiver isn't polled unless tokio happens to move it to another thread (which is at tokio's decretion but not guaranteed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution is likely, I guess, either to change resolve
in the Discovery trait to return a impl Future
, or to spawn a tokio task here that sends the message with send_async
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to check if the sender is full before sending, and return an error if it is, rather than getting blocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this block? If the consumer of the stream is not sufficiently fast in consuming the events, right?
It is a stream of results. So we could check the buffer capacity. If it is less than 2, send an error. If it is less than 1 (would block), discard the event.
That way a slow receiver will get a stream
event
event
error(lagged)
event
... so they know about the issue and we are guaranteed not to block!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes more sense to me to spawn a task to wait for the send_async
to complete. Returning an error in the stream would mean the discovery service would need to start handling an error about lag (which, to me, ultimately means "wait until the next event" instead of just waiting for the next event).
c4699c0
to
cd20cf4
Compare
When we create a discoverer, supply the ip class based on the addresses we are given to announce. If we are given ipv4 & ipv6 addrs, create a discoverer with both. If we are given no addresses to announce, default to only binding on ipv4, just to be safe.
7774cf7
to
896c652
Compare
/netsim branch arqu/netsim_node_not_found |
|
## Description Using [swarm-discovery](https://github.com/rkuhn/swarm-discovery), this PR implements a discovery service for `LocalSwarmDiscovery`, allowing nodes on the same local network to discover each other without using the outside internet. closes #2354 ## Notes & open questions 1) Is `LocalNodeDiscovery` too much of a mouthful? `swarm-discovery` says, of itself, "This library offers a lightweight discovery service based on mDNS." Soooo, how comfortable are we just calling this `mdns`? edit: calling this `LocalSwarmDiscovery` & not `mdns` 2) There are some const to bikeshed: ```rust /// The n0 local node discovery name const N0_MDNS_SWARM: &str = "iroh.local.node.discovery"; edit: now N0_LOCAL_SWARM: &str = "iroh.loca.swarm" /// Provenance string const PROVENANCE: &str = "local.node.discovery"; edit: now "local.swarm.discovery" /// How long we will wait before we stop sending discovery items const DISCOVERY_DURATION: Duration = Duration::from_secs(10); ``` 3) For the `local_swarm_discovery` example, I pulled some code from `iroh-cli` into a new `iroh-progress`, so that I could re-use some progress terminal output it in the example. `iroh-progress` might not be the right name, but what do we think of a separate crate for putting in reusable UI-ish pieces. If we aren't on board with this, I can just copy and paste the logic in for now & remove the crate. edit: Removed this in favor of copy and pasting the code 4) I've added `LocalSwarmDiscovery` as a default discovery service, I'm assuming that's what we are going for here? ## Change checklist - [x] Self-review. - [x] tests - [x] Documentation updates if relevant.
Description
Using swarm-discovery, this PR implements a discovery service for
LocalSwarmDiscovery
, allowing nodes on the same local network to discover each other without using the outside internet.closes #2354
Notes & open questions
Is
LocalNodeDiscovery
too much of a mouthful?swarm-discovery
says, of itself, "This library offers a lightweight discovery service based on mDNS." Soooo, how comfortable are we just calling thismdns
?edit: calling this
LocalSwarmDiscovery
& notmdns
There are some const to bikeshed:
For the
local_swarm_discovery
example, I pulled some code fromiroh-cli
into a newiroh-progress
, so that I could re-use some progress terminal output it in the example.iroh-progress
might not be the right name, but what do we think of a separate crate for putting in reusable UI-ish pieces. If we aren't on board with this, I can just copy and paste the logic in for now & remove the crate.edit: Removed this in favor of copy and pasting the code
I've added
LocalSwarmDiscovery
as a default discovery service, I'm assuming that's what we are going for here?Change checklist