Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Custom ProtocolsHandler and multiple protocols.
  1. Implement a custom ProtocolsHandler instead of using
     the OneShotHandler for better control and error handling.
     In particular, all request/response sending/receiving is
     kept in the substreams upgrades and thus the background
     task of a connection.
  2. Support multiple protocols (usually protocol versions)
     with a single `RequestResponse` instance, with
     configurable inbound/outbound support.
  • Loading branch information
Roman S. Borschel committed Jun 8, 2020
commit 89ea70af66bda8a3834e773f5a8e7e595c1f7ecd
2 changes: 1 addition & 1 deletion protocols/request-response/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ keywords = ["peer-to-peer", "libp2p", "networking"]
categories = ["network-programming", "asynchronous"]

[dependencies]
bytes = "0.5"
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"
Expand Down
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;
/// 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