refactor: streamline local endpoint discovery #1847
Conversation
iroh-net/src/magic_endpoint.rs
Outdated
| @@ -380,6 +352,11 @@ impl MagicEndpoint { | |||
| self.msock.local_endpoints().await | |||
| } | |||
|
|
|||
| /// Returns the next non empty version of `local_endpoints`. | |||
| pub async fn ensure_local_endpoints(&self) -> Result<Vec<config::Endpoint>> { | |||
There was a problem hiding this comment.
why not build this into the local_endpoints() call instead of adding a new API? The first time it is called it could ensure that it waits - it already is async.
The address-related API of MagicEndpoint is already large, and this addition does not make it clearer. This is supposed to be the high-level-easy-to-use API...
There was a problem hiding this comment.
yes, agreed, fixed
|
How is progress here? This is kind of a blocker for deltachat/deltachat-core-rust#5041 |
iroh-gossip/examples/chat.rs
Outdated
| @@ -106,35 +105,17 @@ async fn main() -> anyhow::Result<()> { | |||
| // init a cell that will hold our gossip handle to be used in endpoint callbacks | |||
| let gossip_cell: OnceCell<Gossip> = OnceCell::new(); | |||
There was a problem hiding this comment.
Indeed, looks like it is .set() and then dropped.
This is a nicer API, but dc core PR can use existing one? The real blocker is #1831 because we don't want to depend on central DERP servers, but no rush, we are not planning to merge deltachat/deltachat-core-rust#5041 this month. |
To avoid the issues seen when setting up an `iroh-net` node, this allows to dynamically subscribe to endpoint changes. It also removes the other callbacks on the magicsock, which where unused.
dignifiedquire
force-pushed
the
refactor-improved-startup
branch
from
891077d to
6b3d8af
Compare
| { | ||
| // check if we have some value already | ||
| let current_value = self.inner.endpoints.read(); | ||
| if !current_value.is_empty() { |
There was a problem hiding this comment.
is it ever possible this will never contain an endpoint? That would probably already be a bug so just fine to hang indefinitely?
There was a problem hiding this comment.
it is expected on startup for this to be empty
| .update(DiscoveredEndpoints::new(eps)) | ||
| .is_ok(); | ||
| if updated { | ||
| let eps = self.inner.endpoints.read(); |
There was a problem hiding this comment.
I think this code is fine. But seeing .read() here did make me go round and check all the docs on what exactly we're reading, who we're notifying and why we must have a lock. Instinctively I would have cloned the endpoints before storing and then worked of the clone. It's much of a muchness though.
There was a problem hiding this comment.
I did it this way, to avoid having to clone in all cases, and only if we actually need to do so
| } | ||
|
|
||
| /// Waits for local endpoints to change and returns the new ones. | ||
| pub async fn local_endpoints_change(&self) -> Result<Vec<config::Endpoint>> { |
There was a problem hiding this comment.
I can't help but bikeshed, something about _change doesn't feel right. It's almost like you want to return a Watchable<Vec<config::Endpoint>>, but that would expose a foreign crate on the API which is no good.
Maybe just local_endpoints_updated() is better already? I think the tense of change is also bothering me.
There was a problem hiding this comment.
both changed and updated sound to me like it should return a boolean to me
There was a problem hiding this comment.
maybe next_local_endpoints, matching the stream api, as it behaves similarly like that
There was a problem hiding this comment.
next_local_endpoints gives me the expectation of an Option... naming is hard. maybe putting the verb at the front is better? updated_local_endpoints
let's not make this a blocker, it's not worth it. and whatever we'll come up with now probably won't be great anyway. maybe someone will realise a great name while using it and then we can change the name as long as we're not 1.0.
To avoid the issues seen when setting up an `iroh-net` node, this allows to dynamically subscribe to endpoint changes. It also removes the other callbacks on the magicsock, which where unused.
To avoid the issues seen when setting up an
iroh-netnode, this allows to dynamically subscribe to endpoint changes. It also removes the other callbacks on the magicsock, which where unused.