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

Jump to bottom

dcutr: fix clippy lints #2772

Merged
merged 3 commits into from
Jul 28, 2022
Merged

dcutr: fix clippy lints #2772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13fb943
dcutr: fix clippy lints
elenaf9 Jul 24, 2022
ba49120
Merge branch 'master' into dcutr/fix-clippy
elenaf9 Jul 26, 2022
0994710
Merge branch 'master' into dcutr/fix-clippy
elenaf9 Jul 27, 2022
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
61 changes: 28 additions & 33 deletions protocols/dcutr/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub enum UpgradeError {
Handler(ConnectionHandlerUpgrErr<void::Void>),
}

#[derive(Default)]
pub struct Behaviour {
/// Queue of actions to return when polled.
queued_actions: VecDeque<ActionBuilder>,
Expand Down Expand Up @@ -145,40 +146,35 @@ impl NetworkBehaviour for Behaviour {
handler: Self::ConnectionHandler,
_error: &DialError,
) {
match handler {
handler::Prototype::DirectConnection {
relayed_connection_id,
role: handler::Role::Initiator { attempt },
} => {
let peer_id =
peer_id.expect("Peer of `Prototype::DirectConnection` is always known.");
if attempt < MAX_NUMBER_OF_UPGRADE_ATTEMPTS {
self.queued_actions.push_back(ActionBuilder::Connect {
if let handler::Prototype::DirectConnection {
relayed_connection_id,
role: handler::Role::Initiator { attempt },
} = handler
{
let peer_id = peer_id.expect("Peer of `Prototype::DirectConnection` is always known.");
if attempt < MAX_NUMBER_OF_UPGRADE_ATTEMPTS {
self.queued_actions.push_back(ActionBuilder::Connect {
peer_id,
handler: NotifyHandler::One(relayed_connection_id),
attempt: attempt + 1,
});
} else {
self.queued_actions.extend([
NetworkBehaviourAction::NotifyHandler {
peer_id,
handler: NotifyHandler::One(relayed_connection_id),
attempt: attempt + 1,
});
} else {
self.queued_actions.extend([
NetworkBehaviourAction::NotifyHandler {
peer_id,
handler: NotifyHandler::One(relayed_connection_id),
event: Either::Left(
handler::relayed::Command::UpgradeFinishedDontKeepAlive,
),
}
.into(),
NetworkBehaviourAction::GenerateEvent(
Event::DirectConnectionUpgradeFailed {
remote_peer_id: peer_id,
error: UpgradeError::Dial,
},
)
.into(),
]);
}
event: Either::Left(
handler::relayed::Command::UpgradeFinishedDontKeepAlive,
),
}
.into(),
NetworkBehaviourAction::GenerateEvent(Event::DirectConnectionUpgradeFailed {
remote_peer_id: peer_id,
error: UpgradeError::Dial,
})
.into(),
]);
}
_ => {}
}
}

Expand Down Expand Up @@ -324,7 +320,6 @@ impl NetworkBehaviour for Behaviour {

/// A [`NetworkBehaviourAction`], either complete, or still requiring data from [`PollParameters`]
/// before being returned in [`Behaviour::poll`].
#[allow(clippy::large_enum_variant)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I always allow this one because the Box is quite the ergonomic hit when pattern matching and the lint desc. also says that one would need to measure the performance benefit gained from boxing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that in some cases it makes more sense to allow this lint rather than using a Box.
That being said, in this case here we pass around the inbound_connect quite a bit without using it, and for all enums it appeared on this error was thrown. We can wrap it once in a Box and pass that around instead, without any ergonomic hit. The only place where inbound_connect is used is in

rust-libp2p/protocols/dcutr/src/handler/relayed.rs

Lines 371 to 373 in f85a990

if let Some(Poll::Ready(result)) = self.inbound_connect.as_mut().map(|f| f.poll_unpin(cx)) {
self.inbound_connect = None;

where we have to use AsMut anyway to access the inner future.

enum ActionBuilder {
Done(NetworkBehaviourAction<Event, handler::Prototype>),
Connect {
Expand All @@ -333,7 +328,7 @@ enum ActionBuilder {
peer_id: PeerId,
},
AcceptInboundConnect {
inbound_connect: protocol::inbound::PendingConnect,
inbound_connect: Box<protocol::inbound::PendingConnect>,
handler: NotifyHandler,
peer_id: PeerId,
},
Expand Down
12 changes: 6 additions & 6 deletions protocols/dcutr/src/handler/relayed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub enum Command {
},
AcceptInboundConnect {
obs_addrs: Vec<Multiaddr>,
inbound_connect: protocol::inbound::PendingConnect,
inbound_connect: Box<protocol::inbound::PendingConnect>,
},
/// Upgrading the relayed connection to a direct connection either failed for good or succeeded.
/// There is no need to keep the relayed connection alive for the sake of upgrading to a direct
Expand Down Expand Up @@ -76,7 +76,7 @@ impl fmt::Debug for Command {

pub enum Event {
InboundConnectRequest {
inbound_connect: protocol::inbound::PendingConnect,
inbound_connect: Box<protocol::inbound::PendingConnect>,
remote_addr: Multiaddr,
},
InboundNegotiationFailed {
Expand Down Expand Up @@ -201,7 +201,7 @@ impl ConnectionHandler for Handler {
};
self.queued_events.push_back(ConnectionHandlerEvent::Custom(
Event::InboundConnectRequest {
inbound_connect,
inbound_connect: Box::new(inbound_connect),
remote_addr,
},
));
Expand Down Expand Up @@ -245,9 +245,10 @@ impl ConnectionHandler for Handler {
inbound_connect,
obs_addrs,
} => {
if let Some(_) = self
if self
.inbound_connect
.replace(inbound_connect.accept(obs_addrs).boxed())
.is_some()
{
log::warn!(
"New inbound connect stream while still upgrading previous one. \
Expand Down Expand Up @@ -337,8 +338,7 @@ impl ConnectionHandler for Handler {
_ => {
// Anything else is considered a fatal error or misbehaviour of
// the remote peer and results in closing the connection.
self.pending_error =
Some(error.map_upgrade_err(|e| e.map_err(|e| EitherError::B(e))));
self.pending_error = Some(error.map_upgrade_err(|e| e.map_err(EitherError::B)));
}
}
}
Expand Down