feat(id): keep a track of connections using a connection ID within qp2p #295
Conversation
| @@ -476,8 +478,8 @@ impl Endpoint { | |||
| } | |||
|
|
|||
| /// Get an existing connection for the peer address. | |||
| pub(crate) async fn get_connection(&self, peer_addr: &SocketAddr) -> Option<Connection> { | |||
| if let Some((conn, guard)) = self.connection_pool.get(peer_addr).await { | |||
| pub(crate) async fn get_connection(&self, peer_addr: &SocketAddr) -> Option<Connection<I>> { | |||
There was a problem hiding this comment.
we probably need get_connection_by_id too no?
In Safe we'd want to avoid any referencing via SockerAddr
There was a problem hiding this comment.
I've left some non-functional nitpicks.
Another thing I wondered would be whether we should give I a default type? Perhaps impl Id for u64 and I = u64 (maybe we could even remove Key?).
May not be worth it though, since this would already be a breaking change with the method renames.
| .range(Key::min(*addr)..=Key::max(*addr)) | ||
| .next() | ||
| .is_some() | ||
| } | ||
|
|
||
| #[allow(unused)] |
There was a problem hiding this comment.
Nitpick: This should be unnecessary for a pub fn.
src/connection_pool.rs
Outdated
| @@ -14,69 +14,90 @@ use tokio::sync::RwLock; | |||
| // Pool for keeping open connections. Pooled connections are associated with a `ConnectionRemover` | |||
| // which can be used to remove them from the pool. | |||
| #[derive(Clone)] | |||
| pub(crate) struct ConnectionPool { | |||
| store: Arc<RwLock<Store>>, | |||
| pub(crate) struct ConnectionPool<T: Id> { | |||
There was a problem hiding this comment.
Nitpick: we probably should be consistent with the type parameter name. We're mostly using I: Id, which I think is a bit better naming.
src/connection_pool.rs
Outdated
| .range(Key::min(*addr)..=Key::max(*addr)) | ||
| .next() | ||
| .is_some() | ||
| } | ||
|
|
||
| #[allow(unused)] | ||
| pub async fn has_id(&self, id: &T) -> bool { |
There was a problem hiding this comment.
perhaps we should rename it to has_conn_id ? and also perhaps have the methods like this one to expect an arg of type &dyn ConnId rather than having a generic on the whole structs?
src/connection_pool.rs
Outdated
| id_gen: IdGen, | ||
| } | ||
|
|
||
| /// Unique key identifying a connection. Two connections will always have distict keys even if they | ||
| /// have the same socket address. | ||
| pub trait Id: |
There was a problem hiding this comment.
How about naming it ConnId since it's what it is?
BREAKING CHANGE: introduces generic type to Qp2p and Endpoint
- wraps the return type of the generate function in a Result - updates tests and doc-tests - implements ConnId for XorName
No description provided.