-
Notifications
You must be signed in to change notification settings - Fork 23
improve efficiency of propagating address failures #443
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
base: master
Are you sure you want to change the base?
Conversation
fallback_names: Vec<ProtocolName>, | ||
codec: ProtocolCodec, | ||
keep_alive_timeout: Duration, | ||
needs_dial_failure_addresses: bool, | ||
) -> TransportService { |
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.
I would add an explicit enum for this option:
pub enum DialFailureAddresses {
Required,
NotRequired,
}
Such that we can easily follow during the protocol registration the purpose of this flag
src/transport/manager/mod.rs
Outdated
peer, | ||
addresses: addresses.clone(), | ||
}) { | ||
.try_send(make_dial_failure_event(&self.address_reporting_protocols, protocol, peer, addresses.clone())) { |
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.
We could introduce a tiny helper over the protocol context:
impl ProtocolContext {
fn dial_failure(&self, addresses: &[Multiaddr]) -> Vec<Multiaddr> {
match self.dial_failure_mode {
DialFailureMode::Required => addresses.to_vec(),
DialFailureMode::NotRequired => Vec::new(),
}
}
}
let _ = match context.tx.try_send(
InnerTransportEvent::DialFailure {
peer,
addresses: context.dial_failure(addresses)
}
)
This way we avoid cloning the addresses until we need them, while keeping the InnerTransportEvent::DialFailure
event explicit. This will also provide a bit better isolation to avoid free floating functions
const LOG_TARGET: &str = "litep2p::transport-manager"; | ||
|
||
#[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
pub enum DialFailureAddresses { |
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.
nit: We could add some documentation explaining the purpose of this enum
/// Fallback names for the protocol. | ||
pub fallback_names: Vec<ProtocolName>, | ||
|
||
pub dial_failure_mode: DialFailureAddresses, |
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.
nit: Missing docs
src/transport/manager/mod.rs
Outdated
let adresses = context.dial_failure_addresses(&[address.clone()]); | ||
|
||
match context.tx.try_send(make_dial_failure_event(peer, adresses.clone())) { |
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.
nit: Is there a way to avoid the cloning here? Maybe changing the signature of the fn to take a & Multiaddr
?
I would also inline the make_dial_failure_event
:
match context.tx.try_send(InnerTransportEvent::DialFailure { peer, addresses })
Instead of cloning the addreses here (addresses.clone()
), we could recreate the addresses on error
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.
i have changed to this inline way, but can't still do without cloning
closes #409