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

Add libp2p-request-response protocol. #1596

Merged
merged 21 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ members = [
"protocols/noise",
"protocols/ping",
"protocols/plaintext",
"protocols/request-response",
"protocols/secio",
"swarm",
"transports/dns",
Expand Down
13 changes: 8 additions & 5 deletions core/src/upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ pub use self::{
///
/// # Context
///
/// In situations where we provide a list of protocols that we support, the elements of that list are required to
/// implement the [`ProtocolName`] trait.
/// In situations where we provide a list of protocols that we support,
/// the elements of that list are required to implement the [`ProtocolName`] trait.
///
/// Libp2p will call the [`ProtocolName::protocol_name`] trait method on each element of that list, and transmit the
/// returned value on the network. If the remote accepts a given protocol, the element serves as the return value of
/// the function that performed the negotiation.
/// Libp2p will call [`ProtocolName::protocol_name`] on each element of that list, and transmit the
/// returned value on the network. If the remote accepts a given protocol, the element
/// serves as the return value of the function that performed the negotiation.
///
/// # Example
///
Expand All @@ -118,6 +118,9 @@ pub use self::{
///
pub trait ProtocolName {
/// The protocol name as bytes. Transmitted on the network.
///
/// **Note:** Valid protocol names must start with `/` and
/// not exceed 140 bytes in length.
fn protocol_name(&self) -> &[u8];
}

Expand Down
11 changes: 6 additions & 5 deletions protocols/floodsub/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use libp2p_swarm::{
NetworkBehaviourAction,
PollParameters,
ProtocolsHandler,
OneShotEvent,
OneShotHandler,
NotifyHandler,
DialPeerCondition,
Expand Down Expand Up @@ -237,7 +238,7 @@ impl Floodsub {
}

impl NetworkBehaviour for Floodsub {
type ProtocolsHandler = OneShotHandler<FloodsubProtocol, FloodsubRpc, InnerMessage>;
type ProtocolsHandler = OneShotHandler<FloodsubProtocol, FloodsubRpc, InnerMessage, ()>;
type OutEvent = FloodsubEvent;

fn new_handler(&mut self) -> Self::ProtocolsHandler {
Expand Down Expand Up @@ -287,12 +288,12 @@ impl NetworkBehaviour for Floodsub {
&mut self,
propagation_source: PeerId,
_connection: ConnectionId,
event: InnerMessage,
event: OneShotEvent<InnerMessage, ()>,
) {
// We ignore successful sends event.
// We ignore successful sends or timeouts.
let event = match event {
InnerMessage::Rx(event) => event,
InnerMessage::Sent => return,
OneShotEvent::Success(InnerMessage::Rx(event)) => event,
OneShotEvent::Success(InnerMessage::Sent) | OneShotEvent::Timeout(()) => return,
};

// Update connected peers topics
Expand Down
24 changes: 24 additions & 0 deletions protocols/request-response/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
[package]
name = "libp2p-request-response"
edition = "2018"
description = "Generic Request/Response Protocols"
version = "0.19.0"
authors = ["Parity Technologies <admin@parity.io>"]
license = "MIT"
repository = "https://github.com/libp2p/rust-libp2p"
keywords = ["peer-to-peer", "libp2p", "networking"]
categories = ["network-programming", "asynchronous"]

[dependencies]
futures = "0.3.1"
libp2p-core = { version = "0.19.0", path = "../../core" }
libp2p-swarm = { version = "0.19.0", path = "../../swarm" }
smallvec = "1.4"
wasm-timer = "0.2"

[dev-dependencies]
async-std = "< 1.6"
romanb marked this conversation as resolved.
Show resolved Hide resolved
libp2p-noise = { version = "0.19.0", path = "../noise" }
libp2p-tcp = { version = "0.19.0", path = "../../transports/tcp", features = ["async-std"] }
libp2p-yamux = { version = "0.19.0", path = "../../muxers/yamux" }
rand = "0.7"
64 changes: 64 additions & 0 deletions protocols/request-response/src/codec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright 2020 Parity Technologies (UK) Ltd.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the "Software"),
// to deal in the Software without restriction, including without limitation
// the rights to use, copy, modify, merge, publish, distribute, sublicense,
// and/or sell copies of the Software, and to permit persons to whom the
// Software is furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.

pub use libp2p_core::ProtocolName;

use futures::{prelude::*, future::BoxFuture};
use std::io;

/// A `RequestResponseCodec` 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.
pub trait RequestResponseCodec {
/// The type of protocol(s) or protocol versions being negotiated.
type Protocol: ProtocolName + Send + Sync + Clone;
romanb marked this conversation as resolved.
Show resolved Hide resolved
/// The type of inbound and outbound requests.
type Request: Send + Clone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a requirement on inbound events to any ProtocolsHandler (due to the general possibility of NotifyHandler::All).

Copy link
Contributor

Choose a reason for hiding this comment

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

Where in the type-system is this enforced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ExpandedSwarm needs Clone on TInEvent since notify_all needs Clone on TInEvent. Is that what you were asking for?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that removing Clone does not cause compilation to fail. Should the bound not be required by the ProtocolsHandler::InEvent type?

Copy link
Contributor Author

@romanb romanb Jun 25, 2020

Choose a reason for hiding this comment

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

I noticed that removing Clone does not cause compilation to fail.

Indeed, that is true, the bound here does not seem necessary. Here InEvent is struct RequestProtocol<TCodec> which has a field of type TCodec::Request together with [derive(Clone)]. So certainly RequestProtocol cannot be Clone without TCodec::Request being Clone (and sure enough the included test with a ping/pong protocol complains firstly about the missing Clone for struct Ping due to the requirement for TInEvent: Clone from the Swarm). So the TCodec::Request: Clone bound is probably "implied" by the Clone derive on RequestProtocol.

Should the bound not be required by the ProtocolsHandler::InEvent type?

You mean as an alternative to putting the bound in the ExpandedSwarm impl? That would probably work - we have many such cases where the Swarm or Network is not really useable without certain constraints on some associated types, yet the bounds are placed on methods or impls of the Swarm or Network, instead of on the associated type. I personally would prefer the bounds to be directly on the associated types in these cases, but it is probably not something for this PR.

In any case, I will remove the seemingly unnecessary Clone constraint here, thanks for pointing that out.

/// The type of inbound and outbound responses.
type Response: Send;

/// Reads a request from the given I/O stream according to the
/// negotiated protocol.
fn read_request<'a, T>(&mut self, protocol: &Self::Protocol, io: &'a mut T)
-> BoxFuture<'a, Result<Self::Request, io::Error>>
where
T: AsyncRead + Unpin + Send;

/// Reads a response from the given I/O stream according to the
/// negotiated protocol.
fn read_response<'a, T>(&mut self, protocol: &Self::Protocol, io: &'a mut T)
-> BoxFuture<'a, Result<Self::Response, io::Error>>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using async-trait for this trait and writing:

async fn read_response<T>(&mut self, protocol: &Self::Protocol, io: &mut T) -> io::Result<Self::Response>

Copy link
Member

@tomaka tomaka Jun 25, 2020

Choose a reason for hiding this comment

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

I'm personally not a fan of async-trait. It hides things from you for a very minimal gain. No strong opinion on this however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I wasn't sure how controversial it would be and was even curious if someone would suggest it. While I have no strong preference and it is a relatively small convenience, I do see the appeal in getting syntactic uniformity for async (i.e. future-returning) methods, whether in traits or not. In terms of hiding things I personally don't think it makes the situation worse because the code expands in a very similar way to that of the existing built-in async fns for non-trait methods (subject to the inherent limitations, of course), which one needs to understand in any case. So I have async-trait a try: 9c6e6b4. If there are strong objections, please let me know.

where
T: AsyncRead + Unpin + Send;

/// Writes a request to the given I/O stream according to the
/// negotiated protocol.
fn write_request<'a, T>(&mut self, protocol: &Self::Protocol, io: &'a mut T, req: Self::Request)
-> BoxFuture<'a, Result<(), io::Error>>
where
T: AsyncWrite + Unpin + Send;

/// Writes a response to the given I/O stream according to the
/// negotiated protocol.
fn write_response<'a, T>(&mut self, protocol: &Self::Protocol, io: &'a mut T, res: Self::Response)
-> BoxFuture<'a, Result<(), io::Error>>
where
T: AsyncWrite + Unpin + Send;
}
Loading