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

refactor(request-response): revise public API to follow naming convention #3159

Merged
merged 31 commits into from
Dec 13, 2022
Merged

refactor(request-response): revise public API to follow naming convention #3159

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
5ec3449
request-response: rename RequestResponseMessage to Message
jxs Nov 22, 2022
744c2fb
request-response: rename RequestResponseHandler to Handler,
jxs Nov 22, 2022
d0c69a8
request-response: rename RequestResponseCodec to Codec
jxs Nov 22, 2022
bf3c663
request-response: rename RequestResponseEvent to Event
jxs Nov 22, 2022
abd326c
request-response: rename RequestResponseConfig to Config
jxs Nov 22, 2022
8dcf4b5
request-response: rename RequestResponse to Behaviour
jxs Nov 22, 2022
f5fcc29
request-response: remove [doc(hidden)] from Handler
jxs Nov 22, 2022
6d998d4
review: rename HandlerEvent to handler::Event
jxs Nov 24, 2022
3693a79
review: re-create deprcated RequestResponseCodec.
jxs Nov 24, 2022
d2880cc
review: improve request-response Handler doc wording.
jxs Nov 24, 2022
0e42929
Merge github.com:libp2p/rust-libp2p into rename-request-response
jxs Nov 24, 2022
00465e4
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 1, 2022
57bb47f
review: Update CHANGELOG.md to reflect libp2p-request-response 0.23 r…
jxs Dec 1, 2022
11ca1df
review: update version on missing places
jxs Dec 4, 2022
637a32d
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 4, 2022
dabd8bc
review: apply Thomas wording suggestions
jxs Dec 4, 2022
82c0e85
review: blanket Codec impl for types that impl RequestResponseCodec.
jxs Dec 4, 2022
9813119
review: rename pending HandlerEvent references.
jxs Dec 5, 2022
0b5eb4c
review: make blanket Codec implementation #[async_trait].
jxs Dec 5, 2022
9be0603
review: improve wording on doc
jxs Dec 5, 2022
84dd335
review: update release to patch instead of minor
jxs Dec 9, 2022
183f88d
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 9, 2022
1f3fbab
review: rename dangling 0.24 references.
jxs Dec 9, 2022
533ae05
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 12, 2022
223ec5f
review: update deprecated messages back to 0.24 version
jxs Dec 12, 2022
12094b4
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 12, 2022
7cd250c
merge: replace removed webrtc smoke test functions
jxs Dec 12, 2022
c030232
review: clippy
jxs Dec 13, 2022
fb07b18
Merge branch 'master' of github.com:libp2p/rust-libp2p into rename-re…
jxs Dec 13, 2022
a613a94
review: fix docs
jxs Dec 13, 2022
6db0668
Merge branch 'master' into rename-request-response
mergify[bot] Dec 13, 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
5 changes: 2 additions & 3 deletions examples/file-sharing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ mod network {
use libp2p::kad::{GetProvidersOk, Kademlia, KademliaEvent, QueryId, QueryResult};
use libp2p::multiaddr::Protocol;
use libp2p::request_response::{
self, ProtocolSupport, RequestId, RequestResponse, RequestResponseCodec,
RequestResponseEvent, ResponseChannel,
self, ProtocolSupport, RequestId, RequestResponse, RequestResponseEvent, ResponseChannel,
};
use libp2p::swarm::{ConnectionHandlerUpgrErr, NetworkBehaviour, Swarm, SwarmEvent};
use std::collections::{hash_map, HashMap, HashSet};
Expand Down Expand Up @@ -663,7 +662,7 @@ mod network {
}

#[async_trait]
impl RequestResponseCodec for FileExchangeCodec {
impl request_response::Codec for FileExchangeCodec {
type Protocol = FileExchangeProtocol;
type Request = FileRequest;
type Response = FileResponse;
Expand Down
4 changes: 2 additions & 2 deletions protocols/autonat/src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::structs_proto;
use async_trait::async_trait;
use futures::io::{AsyncRead, AsyncWrite, AsyncWriteExt};
use libp2p_core::{upgrade, Multiaddr, PeerId};
use libp2p_request_response::{ProtocolName, RequestResponseCodec};
use libp2p_request_response::{self as request_response, ProtocolName};
use prost::Message;
use std::{convert::TryFrom, io};

Expand All @@ -42,7 +42,7 @@ impl ProtocolName for AutoNatProtocol {
pub struct AutoNatCodec;

#[async_trait]
impl RequestResponseCodec for AutoNatCodec {
impl request_response::Codec for AutoNatCodec {
type Protocol = AutoNatProtocol;
type Request = DialRequest;
type Response = DialResponse;
Expand Down
4 changes: 2 additions & 2 deletions protocols/request-response/src/codec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ use async_trait::async_trait;
use futures::prelude::*;
use std::io;

/// A `RequestResponseCodec` defines the request and response types
/// A `Codec` defines the request and response types
/// for a [`RequestResponse`](crate::RequestResponse) protocol or
/// protocol family and how they are encoded / decoded on an I/O stream.
#[async_trait]
pub trait RequestResponseCodec {
pub trait Codec {
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
/// The type of protocol(s) or protocol versions being negotiated.
type Protocol: ProtocolName + Send + Clone;
/// The type of inbound and outbound requests.
Expand Down
12 changes: 6 additions & 6 deletions protocols/request-response/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

mod protocol;

use crate::codec::RequestResponseCodec;
use crate::codec::Codec;
use crate::{RequestId, EMPTY_QUEUE_SHRINK_THRESHOLD};

use libp2p_swarm::handler::{
Expand Down Expand Up @@ -58,7 +58,7 @@ pub type RequestResponseHandler<TCodec> = Handler<TCodec>;
#[doc(hidden)]
pub struct Handler<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
/// The supported inbound protocols.
inbound_protocols: SmallVec<[TCodec::Protocol; 2]>,
Expand Down Expand Up @@ -96,7 +96,7 @@ where

impl<TCodec> Handler<TCodec>
where
TCodec: RequestResponseCodec + Send + Clone + 'static,
TCodec: Codec + Send + Clone + 'static,
{
pub(super) fn new(
inbound_protocols: SmallVec<[TCodec::Protocol; 2]>,
Expand Down Expand Up @@ -205,7 +205,7 @@ pub type RequestResponseHandlerEvent<TCodec> = HandlerEvent<TCodec>;
#[doc(hidden)]
pub enum HandlerEvent<TCodec>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would just Event be pushing it too far? :D

Copy link
Contributor

@thomaseizinger thomaseizinger Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In case it isn't I think the handler module should be private so this Event can't clash with the other one.

Copy link
Member

Choose a reason for hiding this comment

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

Intuitively libp2p::request_response::handler::Event reads better than libp2p::request_response::handler::HandlerEvent and would be inline with the convention this pull request is enforcing.

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 makes sense to me, thanks for the suggestion,
So, there is request_response::Event and request_response::handler::Event now. Can you expand on Event's clashing Thomas?
There will still be request_response::RequestResponseHandlerEvent which is deprecated and suggests request_response::handler::Event, if we make handler private how can we export both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah if we make handler private, we can no longer export it. What I meant with clashing is that from the outside, there is really only one event that is interesting for users and it is the NetworkBehaviour's OutEvent.

Given that we are already making this a breaking change, we might as well make handler private. In my opinion, no one should be depending on just the ConnectionHandler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation Thomas. Handler and Event can't be private because NetworkBehaviour::ConnectionBehaviour associated type needs to be public, and by consequence ConnectionHandler's also

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation Thomas. Handler and Event can't be private because NetworkBehaviour::ConnectionBehaviour associated type needs to be public, and by consequence ConnectionHandler's also

Hmm that is news to me! We have a lot of protocols where the ConnectionHandler implementation is completely private to the crate. For example ping:

rust-libp2p/protocols/ping/src/lib.rs

Line 45 in 5755942

mod handler;

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested it locally and things compile if I make the handler module private. However, I'd suggest that we don't do this for now. I think together with #3159 (comment), we can make this completely non-breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for digging this deeper Thomas! Did you understand what is it that with request-response that was making the compiler complain?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue is fairly subtle:

If you have a public type that implements a trait, the compiler enforces that the associated types also need to be public. It does however not check whether the type is within a public or private module in regards to the crate.

In our usecase, this means we can have a pub struct Handler that implements ConnectionHandler but define it within a crate-private module handler. This may or may not be a bug in Rust but I think one can argue that it makes sense:

The API contract promised to the user is that Behaviour implements NetworkBehaviour. NetworkBehaviour enforces that NetworkBehaviour::ConnectionHandler implements IntoConnectionHandler. The user can refer to this type as <Behaviour::ConnectionHandler as IntoConnectionHandler> and call all public APIs on that. They don't need to know the concrete type that implements the functionality. This way, only the minimal promised API is actually exposed.

where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
/// A request has been received.
Request {
Expand Down Expand Up @@ -235,7 +235,7 @@ where
InboundUnsupportedProtocols(RequestId),
}

impl<TCodec: RequestResponseCodec> fmt::Debug for HandlerEvent<TCodec> {
impl<TCodec: Codec> fmt::Debug for HandlerEvent<TCodec> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
HandlerEvent::Request {
Expand Down Expand Up @@ -283,7 +283,7 @@ impl<TCodec: RequestResponseCodec> fmt::Debug for HandlerEvent<TCodec> {

impl<TCodec> ConnectionHandler for Handler<TCodec>
where
TCodec: RequestResponseCodec + Send + Clone + 'static,
TCodec: Codec + Send + Clone + 'static,
{
type InEvent = RequestProtocol<TCodec>;
type OutEvent = HandlerEvent<TCodec>;
Expand Down
16 changes: 8 additions & 8 deletions protocols/request-response/src/handler/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
//! receives a request and sends a response, whereas the
//! outbound upgrade send a request and receives a response.

use crate::codec::RequestResponseCodec;
use crate::codec::Codec;
use crate::RequestId;

use futures::{channel::oneshot, future::BoxFuture, prelude::*};
Expand Down Expand Up @@ -67,7 +67,7 @@ impl ProtocolSupport {
#[derive(Debug)]
pub struct ResponseProtocol<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
pub(crate) codec: TCodec,
pub(crate) protocols: SmallVec<[TCodec::Protocol; 2]>,
Expand All @@ -78,7 +78,7 @@ where

impl<TCodec> UpgradeInfo for ResponseProtocol<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
type Info = TCodec::Protocol;
type InfoIter = smallvec::IntoIter<[Self::Info; 2]>;
Expand All @@ -90,7 +90,7 @@ where

impl<TCodec> InboundUpgrade<NegotiatedSubstream> for ResponseProtocol<TCodec>
where
TCodec: RequestResponseCodec + Send + 'static,
TCodec: Codec + Send + 'static,
{
type Output = bool;
type Error = io::Error;
Expand Down Expand Up @@ -132,7 +132,7 @@ where
/// Sends a request and receives a response.
pub struct RequestProtocol<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
pub(crate) codec: TCodec,
pub(crate) protocols: SmallVec<[TCodec::Protocol; 2]>,
Expand All @@ -142,7 +142,7 @@ where

impl<TCodec> fmt::Debug for RequestProtocol<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("RequestProtocol")
Expand All @@ -153,7 +153,7 @@ where

impl<TCodec> UpgradeInfo for RequestProtocol<TCodec>
where
TCodec: RequestResponseCodec,
TCodec: Codec,
{
type Info = TCodec::Protocol;
type InfoIter = smallvec::IntoIter<[Self::Info; 2]>;
Expand All @@ -165,7 +165,7 @@ where

impl<TCodec> OutboundUpgrade<NegotiatedSubstream> for RequestProtocol<TCodec>
where
TCodec: RequestResponseCodec + Send + 'static,
TCodec: Codec + Send + 'static,
{
type Output = TCodec::Response;
type Error = io::Error;
Expand Down
12 changes: 6 additions & 6 deletions protocols/request-response/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
//! request/response protocol or protocol family, whereby each request is
//! sent over a new substream on a connection. `RequestResponse` is generic
//! over the actual messages being sent, which are defined in terms of a
//! [`RequestResponseCodec`]. Creating a request/response protocol thus amounts
//! [`Codec`]. Creating a request/response protocol thus amounts
//! to providing an implementation of this trait which can then be
//! given to [`RequestResponse::new`]. Further configuration options are
//! available via the [`RequestResponseConfig`].
Expand All @@ -43,7 +43,7 @@
//!
//! A single [`RequestResponse`] instance can be used with an entire
//! protocol family that share the same request and response types.
//! For that purpose, [`RequestResponseCodec::Protocol`] is typically
//! For that purpose, [`Codec::Protocol`] is typically
//! instantiated with a sum type.
//!
//! ## Limited Protocol Support
Expand All @@ -61,7 +61,7 @@
pub mod codec;
pub mod handler;

pub use codec::{ProtocolName, RequestResponseCodec};
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
pub use codec::{Codec, ProtocolName};
pub use handler::ProtocolSupport;

use futures::channel::oneshot;
Expand Down Expand Up @@ -305,7 +305,7 @@ impl RequestResponseConfig {
/// A request/response protocol for some message codec.
pub struct RequestResponse<TCodec>
where
TCodec: RequestResponseCodec + Clone + Send + 'static,
TCodec: Codec + Clone + Send + 'static,
{
/// The supported inbound protocols.
inbound_protocols: SmallVec<[TCodec::Protocol; 2]>,
Expand Down Expand Up @@ -338,7 +338,7 @@ where

impl<TCodec> RequestResponse<TCodec>
where
TCodec: RequestResponseCodec + Clone + Send + 'static,
TCodec: Codec + Clone + Send + 'static,
{
/// Creates a new `RequestResponse` behaviour for the given
/// protocols, codec and configuration.
Expand Down Expand Up @@ -700,7 +700,7 @@ where

impl<TCodec> NetworkBehaviour for RequestResponse<TCodec>
where
TCodec: RequestResponseCodec + Send + Clone + 'static,
TCodec: Codec + Send + Clone + 'static,
{
type ConnectionHandler = Handler<TCodec>;
type OutEvent = RequestResponseEvent<TCodec::Request, TCodec::Response>;
Expand Down
7 changes: 5 additions & 2 deletions protocols/request-response/tests/ping.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ use libp2p::core::{
Multiaddr, PeerId,
};
use libp2p::noise::NoiseAuthenticated;
use libp2p::request_response::*;
use libp2p::request_response::{
self, InboundFailure, Message, OutboundFailure, ProtocolName, ProtocolSupport, RequestResponse,
RequestResponseConfig, RequestResponseEvent,
};
use libp2p::swarm::{Swarm, SwarmEvent};
use libp2p::tcp;
use libp2p_core::Transport;
Expand Down Expand Up @@ -325,7 +328,7 @@ impl ProtocolName for PingProtocol {
}

#[async_trait]
impl RequestResponseCodec for PingCodec {
impl request_response::Codec for PingCodec {
type Protocol = PingProtocol;
type Request = Ping;
type Response = Pong;
Expand Down
6 changes: 3 additions & 3 deletions transports/webrtc/tests/smoke.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use futures::{
};
use libp2p::core::{identity, muxing::StreamMuxerBox, upgrade, Transport as _};
use libp2p::request_response::{
self, ProtocolName, ProtocolSupport, RequestResponse, RequestResponseCodec,
RequestResponseConfig, RequestResponseEvent,
self, ProtocolName, ProtocolSupport, RequestResponse, RequestResponseConfig,
RequestResponseEvent,
};
use libp2p::swarm::{Swarm, SwarmEvent};
use libp2p::webrtc::tokio as webrtc;
Expand Down Expand Up @@ -407,7 +407,7 @@ impl ProtocolName for PingProtocol {
}

#[async_trait]
impl RequestResponseCodec for PingCodec {
impl request_response::Codec for PingCodec {
type Protocol = PingProtocol;
type Request = Ping;
type Response = Pong;
Expand Down