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.

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

refactor(swarm): remove deprecated inject calls #3264

Merged
merged 16 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
27 changes: 2 additions & 25 deletions protocols/dcutr/src/handler/direct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
//! [`ConnectionHandler`] handling direct connection upgraded through a relayed connection.

use libp2p_core::connection::ConnectionId;
use libp2p_core::upgrade::{DeniedUpgrade, InboundUpgrade, OutboundUpgrade};
use libp2p_core::upgrade::DeniedUpgrade;
use libp2p_swarm::handler::ConnectionEvent;
use libp2p_swarm::{
ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive,
NegotiatedSubstream, SubstreamProtocol,
SubstreamProtocol,
};
use std::task::{Context, Poll};
use void::Void;
Expand Down Expand Up @@ -62,31 +62,8 @@ impl ConnectionHandler for Handler {
SubstreamProtocol::new(DeniedUpgrade, ())
}

fn inject_fully_negotiated_inbound(
&mut self,
_: <Self::InboundProtocol as InboundUpgrade<NegotiatedSubstream>>::Output,
_: Self::InboundOpenInfo,
) {
}

fn inject_fully_negotiated_outbound(
&mut self,
_: <Self::OutboundProtocol as OutboundUpgrade<NegotiatedSubstream>>::Output,
_: Self::OutboundOpenInfo,
) {
}

fn on_behaviour_event(&mut self, _: Self::InEvent) {}

fn inject_dial_upgrade_error(
&mut self,
_: Self::OutboundOpenInfo,
_: ConnectionHandlerUpgrErr<
<Self::OutboundProtocol as OutboundUpgrade<NegotiatedSubstream>>::Error,
>,
) {
}

fn connection_keep_alive(&self) -> KeepAlive {
KeepAlive::No
}
Expand Down
2 changes: 1 addition & 1 deletion protocols/relay/src/priv_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl Behaviour {
}
}
hash_map::Entry::Vacant(_) => {
unreachable!("`inject_connection_closed` for unconnected peer.")
unreachable!("`on_connection_closed` for unconnected peer.")
}
};
}
Expand Down
2 changes: 1 addition & 1 deletion protocols/rendezvous/src/handler/inbound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl SubstreamHandler for Stream {
Stream::PendingRead(Framed::new(substream, RendezvousCodec::default()))
}

fn inject_event(self, event: Self::InEvent) -> Self {
fn on_event(self, event: Self::InEvent) -> Self {
match (event, self) {
(InEvent::RegisterResponse { ttl }, Stream::PendingBehaviour(substream)) => {
Stream::PendingSend(substream, Message::RegisterResponse(Ok(ttl)))
Expand Down
2 changes: 1 addition & 1 deletion protocols/rendezvous/src/handler/outbound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl SubstreamHandler for Stream {
}))
}

fn inject_event(self, event: Self::InEvent) -> Self {
fn on_event(self, event: Self::InEvent) -> Self {
void::unreachable(event)
}

Expand Down
71 changes: 37 additions & 34 deletions protocols/rendezvous/src/substream_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ use futures::future::{self, BoxFuture, Fuse, FusedFuture};
use futures::FutureExt;
use instant::Instant;
use libp2p_core::{InboundUpgrade, OutboundUpgrade, UpgradeInfo};
use libp2p_swarm::handler::{InboundUpgradeSend, OutboundUpgradeSend};
use libp2p_swarm::handler::{ConnectionEvent, FullyNegotiatedInbound, FullyNegotiatedOutbound};
use libp2p_swarm::{
ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive,
NegotiatedSubstream, SubstreamProtocol,
ConnectionHandler, ConnectionHandlerEvent, KeepAlive, NegotiatedSubstream, SubstreamProtocol,
};
use std::collections::{HashMap, VecDeque};
use std::fmt;
Expand All @@ -52,7 +51,7 @@ pub trait SubstreamHandler: Sized {
fn upgrade(open_info: Self::OpenInfo)
-> SubstreamProtocol<PassthroughProtocol, Self::OpenInfo>;
fn new(substream: NegotiatedSubstream, info: Self::OpenInfo) -> Self;
fn inject_event(self, event: Self::InEvent) -> Self;
fn on_event(self, event: Self::InEvent) -> Self;
fn advance(self, cx: &mut Context<'_>) -> Result<Next<Self, Self::OutEvent>, Self::Error>;
}

Expand Down Expand Up @@ -367,35 +366,47 @@ where
TInboundSubstreamHandler::upgrade(())
}

fn inject_fully_negotiated_inbound(
fn on_connection_event(
Copy link
Member Author

Choose a reason for hiding this comment

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

this was missed in the previous #3085 sorry, should I add a new changelog entry CC @mxinden? (there was already one in the previous version mentioning the replacement of the methods for Handler)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is breaking, right? Thus advocating for no changelog.

On a related note, shouldn't our CI have caught this? Shouldn't this have generated a deprecation warning and thus shouldn't CI have failed? Can you investigate this @jxs?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this isn't a breaking change afaik, as they are only called by the Behaviour

On a related note, shouldn't our CI have caught this? Shouldn't this have generated a deprecation warning and thus shouldn't CI have failed?

good point, cargo doesn't trigger deprecation warning on the implementation, only on the invocation IRC, do we have any code triggering calls to the methods?

&mut self,
protocol: <Self::InboundProtocol as InboundUpgradeSend>::Output,
_: Self::InboundOpenInfo,
) {
self.inbound_substreams.insert(
self.next_inbound_substream_id.fetch_and_increment(),
TInboundSubstreamHandler::new(protocol, ()),
);
}

fn inject_fully_negotiated_outbound(
&mut self,
protocol: <Self::OutboundProtocol as OutboundUpgradeSend>::Output,
info: Self::OutboundOpenInfo,
event: ConnectionEvent<
Self::InboundProtocol,
Self::OutboundProtocol,
Self::InboundOpenInfo,
Self::OutboundOpenInfo,
>,
) {
self.outbound_substreams.insert(
self.next_outbound_substream_id.fetch_and_increment(),
TOutboundSubstreamHandler::new(protocol, info),
);
match event {
ConnectionEvent::FullyNegotiatedInbound(FullyNegotiatedInbound {
protocol, ..
}) => {
self.inbound_substreams.insert(
self.next_inbound_substream_id.fetch_and_increment(),
TInboundSubstreamHandler::new(protocol, ()),
);
}
ConnectionEvent::FullyNegotiatedOutbound(FullyNegotiatedOutbound {
protocol,
info,
}) => {
self.outbound_substreams.insert(
self.next_outbound_substream_id.fetch_and_increment(),
TOutboundSubstreamHandler::new(protocol, info),
);
}
// TODO: Handle upgrade errors properly
ConnectionEvent::AddressChange(_)
| ConnectionEvent::ListenUpgradeError(_)
| ConnectionEvent::DialUpgradeError(_) => {}
}
}

fn inject_event(&mut self, event: Self::InEvent) {
fn on_behaviour_event(&mut self, event: Self::InEvent) {
match event {
InEvent::NewSubstream { open_info } => self.new_substreams.push_back(open_info),
InEvent::NotifyInboundSubstream { id, message } => {
match self.inbound_substreams.remove(&id) {
Some(handler) => {
let new_handler = handler.inject_event(message);
let new_handler = handler.on_event(message);

self.inbound_substreams.insert(id, new_handler);
}
Expand All @@ -407,7 +418,7 @@ where
InEvent::NotifyOutboundSubstream { id, message } => {
match self.outbound_substreams.remove(&id) {
Some(handler) => {
let new_handler = handler.inject_event(message);
let new_handler = handler.on_event(message);

self.outbound_substreams.insert(id, new_handler);
}
Expand All @@ -419,14 +430,6 @@ where
}
}

fn inject_dial_upgrade_error(
&mut self,
_: Self::OutboundOpenInfo,
_: ConnectionHandlerUpgrErr<Void>,
) {
// TODO: Handle upgrade errors properly
}

fn connection_keep_alive(&self) -> KeepAlive {
// Rudimentary keep-alive handling, to be extended as needed as this abstraction is used more by other protocols.

Expand Down Expand Up @@ -537,7 +540,7 @@ impl SubstreamHandler for void::Void {
unreachable!("we should never yield a substream")
}

fn inject_event(self, event: Self::InEvent) -> Self {
fn on_event(self, event: Self::InEvent) -> Self {
void::unreachable(event)
}

Expand Down
2 changes: 1 addition & 1 deletion protocols/request-response/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ where
Err(oneshot::Canceled) => {
// The inbound upgrade has errored or timed out reading
// or waiting for the request. The handler is informed
// via `inject_listen_upgrade_error`.
// via `on_connection_event` call with `ConnectionEvent::ListenUpgradeError`.
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions swarm-derive/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
# 0.31.0
# 0.32.0 [unreleased]

- Replace `NetworkBehaviour` Derive macro deprecated `inject_*` method implementations
with the new `on_swarm_event` and `on_connection_handler_event`.
Comment on lines 3 to 4
Copy link
Member

Choose a reason for hiding this comment

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

This entry was previously mistakenly in v0.31.0, right? I.e. we did not actually move to the on_ methods in swarm-derive in v0.31.0 as otherwise our non-breaking-change hack would not have worked, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah exactly, thanks for referring this Max.

See [PR 3011].
See [PR 3011] and [PR 3264].

[PR 3011]: https://github.com/libp2p/rust-libp2p/pull/3011
[PR 3264]: https://github.com/libp2p/rust-libp2p/pull/3264

# 0.31.0

- Add `prelude` configuration option.
The derive-macro generates code that needs to refer to various symbols. See [PR 3055].

- Update `rust-version` to reflect the actual MSRV: 1.60.0. See [PR 3090].

[PR 3011]: https://github.com/libp2p/rust-libp2p/pull/3011
[PR 3055]: https://github.com/libp2p/rust-libp2p/pull/3055
[PR 3090]: https://github.com/libp2p/rust-libp2p/pull/3090

Expand Down
2 changes: 1 addition & 1 deletion swarm-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "libp2p-swarm-derive"
edition = "2021"
rust-version = "1.60.0"
description = "Procedural macros of libp2p-swarm"
version = "0.31.0"
version = "0.32.0"
authors = ["Parity Technologies <admin@parity.io>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
Expand Down
Loading