Add libp2p-request-response protocol.

[romanb]

This is a proposal for #1562. A new protocol crate libp2p-request-response is introduced. From the crate documentation:

//! Generic request/response protocols.
//!
//! ## General Usage
//!
//! [`RequestResponse`] is a `NetworkBehaviour` that implements a generic
//! request/response protocol 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
//! to providing an implementation of this trait that can be
//! given to [`RequestResponse::new`]. Further configuration is available
//! via the [`RequestResponseConfig`].
//!
//! Requests are sent using [`RequestResponse::send_request`] and the
//! responses received as [`RequestResponseMessage::Response`] via
//! [`RequestResponseEvent::Message`].
//!
//! Responses are sent using [`RequestResponse::send_response`] upon
//! receiving a [`RequestResponseMessage::Request`] via
//! [`RequestResponseEvent::Message`].
//!
//! ## One-Way Protocols
//!
//! The implementation supports one-way protocols that do not
//! have responses. In these cases the [`RequestResponseCodec::Response`] can
//! be defined as `()` and [`RequestResponseCodec::read_response`] as well as
//! [`RequestResponseCodec::write_response`] given the obvious implementations.
//! Note that `RequestResponseMessage::Response` will still be emitted,
//! immediately after the request has been sent, since `RequestResponseCodec::read_response`
//! will not actually read anything from the given I/O stream.
//! [`RequestResponse::send_response`] need not be called for one-way protocols.

As can be seen, I went for a dedicated trait instead of having users implement all of UpgradeInfo InboundUpgrade and OutboundUpgrade as I think it is the most convenient and avoids all the repetitive parts associated with implementing these traits.

Note further that I made some changes to the OneShotHandler, most notably introducing the OneShotEvent output type so that ProtocolsHandlerUpgrErr::Timeout no longer results in closing the connection but instead reporting the timeout to the behaviour (this relates somewhat to #1530. I think closing the connection is never what you want on a request timeout - if the connection is actually broken in some way other errors will occur and result in the connection being closed, and if the remote peer is "broken" closing the connection won't help if we just open one again for the next request but just increase connection churn. If this change is nevertheless subject to debate we could introduce a configuration option). Then in order for the behaviour to associate timeouts with outbound requests if necessary, I introduced OneShotOutboundInfo which the outbound protocol upgrade must implement (a blanket implementation for () is given), thus making use of the previously unused OneShotHandler::OutboundOpenInfo.

[tomaka]

Thanks for the work!

From a purely technical perspective, I think that there are two problems that should be fixed:

• It is not possible with this PR to support multiple protocols at the same time. In situations where for example you want to upgrade from a protocol version to the next, it can in my opinion be very useful to be able to handle both versions using the same code, with just an if or match block to handle the differences between versions.
  More practically speaking, we should in my opinion provide not just one but a list of protocols that we are capable of handling, and pass back to the user which protocol was used when a request is received.
  It is similarly not possible, with this PR, to perform requests using one protocol while at the same time not supporting incoming requests using that protocol.

• The implementation of NetworkBehaviour::poll, amongst other things, sends back responses. We've been doing it this way in Substrate too, but I don't think it's a good idea. Sending back a response potentially involves several task wake-ups, encrypting communications, and locking mutexes. I think it would be much preferable to send the response to the ProtocolsHandler for it to perform the send back.

There are a few more "ideological" points, that I disagree with, but am not strongly opposed:

• I don't think add_address and remove_address are a good thing, as maintaining a list of addresses seems orthogonal to requests-responses. I understand the need, but I think we should add that differently, perhaps with a struct that wraps around a NetworkBehaviour in a generic way and adds addresses.

• As the person who is usually doing the publishing on crates.io, I'm not sure that this warrants a new crate. I would have seen it in libp2p-swarm, similar to the OneShotHandler.

• I'm also not sure about RequestResponseCodec. If ProtocolName/InboundUpgrade/OutboundUpgrade are too complicated to implement, then we should make them easier to implement. This is a very naive take, and I think it would cause issues to depend on these traits.

[romanb]

It is not possible with this PR to support multiple protocols at the same time. In situations where for example you want to upgrade from a protocol version to the next, it can in my opinion be very useful to be able to handle both versions using the same code, with just an if or match block to handle the differences between versions.
More practically speaking, we should in my opinion provide not just one but a list of protocols that we are capable of handling, and pass back to the user which protocol was used when a request is received.

The intention here is that a single instance of RequestResponse is a single particular protocol or protocol version. You can always have multiple of these instances, which also allows you to have different request and response types via different RequestResponseCodecs for these instances. I don't currently see how permitting the use of multiple protocol names with a single RequestResponse instance would be an improvement (other than reducing the amount of repeated tracking of connected peers, which is a general problem of the composition of NetworkBehaviours that I think should be addressed separately), but I may be missing something.

I think it would be much preferable to send the response to the ProtocolsHandler for it to perform the send back.

That's not possible with the OneShotHandler though, right? I wanted to avoid a new ProtocolsHandler implementation. So the motivation you're referring to is that the task that polls the NetworkBehaviours is usually rather "heavy" and thus you want to avoid wakeups and potentially expensive computations on that task that could be delegated to the background task of a connection and hence potentially a separate thread?

It is similarly not possible, with this PR, to perform requests using one protocol while at the same time not supporting incoming requests using that protocol.

You can implement RequestResponseCodec::read_request to always resolve with an error (and write_response to be a no-op if desired). However, if all peers do this, then obviously no communication is possible with that protocol, so I'm not sure I understand your use-case.

I don't think add_address and remove_address are a good thing, as maintaining a list of addresses seems orthogonal to requests-responses. I understand the need, but I think we should add that differently, perhaps with a struct that wraps around a NetworkBehaviour in a generic way and adds addresses.

The inherent redundancy in the management of both peer addresses and the connected peers is a known problem for any NetworkBehaviour. I think it is beyond the scope of this PR / issue to address that here. add_address and remove_address are purely opt-in, but they are necessary to offer in order to make RequestResponse fully usable on its own (i.e. without having to combine it with other behaviours in a particular way), which it should be.

As the person who is usually doing the publishing on crates.io, I'm not sure that this warrants a new crate. I would have seen it in libp2p-swarm, similar to the OneShotHandler.

OneShotHandler is just a ProtocolsHandler. RequestResponse is a genuine NetworkBehaviour that offers the convenient implementation of request-response protocols, so I think it really should be its own crate. That the release / publishing process is cumbersome is unfortunate but can be improved (I think there is already an open issue for it).

I'm also not sure about RequestResponseCodec. If ProtocolName/InboundUpgrade/OutboundUpgrade are too complicated to implement, then we should make them easier to implement. This is a very naive take, and I think it would cause issues to depend on these traits.

It is not UpgradeInfo / InboundUpgrade / OutboundUpgrade that are at fault here, it is just that implementing a request-response protocol involves a control-flow pattern for reading/writing requests/responses in these upgrades that is always the same and hence shouldn't need to be repeated all the time - it is the purpose of the generic RequestResponse protocol to take on all the repetitive code so the only thing you have to implement for such a protocol is actually what matters and differs, namely request and response types and how they are encoded/decoded, which is what the contract of RequestResponseCodec requires.

[romanb]

FYI, I'm currently trying out / experimenting with some changes for the two points mentioned below and will report back here later.

1. It is not possible with this PR to support multiple protocols at the same time. [..]

2. The implementation of NetworkBehaviour::poll, amongst other things, sends back responses. We've been doing it this way in Substrate too, but I don't think it's a good idea. [..]

[tomaka]

I don't currently see how permitting the use of multiple protocol names with a single RequestResponse instance would be an improvement (other than reducing the amount of repeated tracking of connected peers, which is a general problem of the composition of NetworkBehaviours that I think should be addressed separately), but I may be missing something.

I admit that I'm raising this point because it is relatively difficulty to compose multiple NetworkBehaviours together at the moment.

That's not possible with the OneShotHandler though, right?

Indeed

So the motivation you're referring to is that the task that polls the NetworkBehaviours is usually rather "heavy" and thus you want to avoid wakeups and potentially expensive computations on that task that could be delegated to the background task of a connection and hence potentially a separate thread?

Yes! In Substrate/Polkadot, the task that polls the NetworkBehaviours is already quite overloaded. It feels a bit stupid to have background tasks specifically dedicated to individual connections if in the end it is the NetworkBehaviour that does the encryption and sends data over the sockets.

You can implement RequestResponseCodec::read_request to always resolve with an error (and write_response to be a no-op if desired). However, if all peers do this, then obviously no communication is possible with that protocol, so I'm not sure I understand your use-case.

It is not the same thing to resolve with an error and to not advertise the protocol at all.

The list of protocols that IntoProtocolsHandler::inbound_protocol returns is captured by the swarm and advertised through the identify protocol.
The quality of this code is a bit questionable, but it's the best compromise that I've found at the time.

In other words, if you return a protocol as part of inbound_protocol, then you advertise it to other nodes. It would then be dishonest to always return an error.

[romanb]

FYI, I'm currently trying out / experimenting with some changes for the two points mentioned below and will report back here later.

1. > It is not possible with this PR to support multiple protocols at the same time. [..]

2. > The implementation of NetworkBehaviour::poll, amongst other things, sends back responses. We've been doing it this way in Substrate too, but I don't think it's a good idea.  [..]

I addressed these two points with this commit and also added support for inbound-only or outbound-only configuration of protocols. An excerpt from the crate documentation:

//! ## Protocol Families
//!
//! 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
//! instantiated with a sum type.
//!
//! ## One-Way Protocols
//!
//! The implementation supports one-way protocols that do not
//! have responses. In these cases the [`RequestResponseCodec::Response`] can
//! be defined as `()` and [`RequestResponseCodec::read_response`] as well as
//! [`RequestResponseCodec::write_response`] given the obvious implementations.
//! Note that `RequestResponseMessage::Response` will still be emitted,
//! immediately after the request has been sent, since `RequestResponseCodec::read_response`
//! will not actually read anything from the given I/O stream.
//! [`RequestResponse::send_response`] need not be called for one-way protocols,
//! i.e. the [`ResponseChannel`] may just be dropped.
//!
//! ## Limited Protocol Support
//!
//! It is possible to only support inbound or outbound requests for
//! a particular protocol. This is achieved by instantiating `RequestResponse`
//! with protocols using [`ProtocolSupport::Inbound`] or
//! [`ProtocolSupport::Outbound`]. Any subset of protocols of a protocol
//! family can be configured in this way. Such protocols will not be
//! advertised during inbound respectively outbound protocol negotiation
//! on the substreams.

The integration test shows a simple example of a protocol implementation used with this behaviour.

[romanb]

Since the code in this PR is now using a custom ProtocolsHandler and not the OneShotHandler, I could revert the changes I made to the OneShotHandler w.r.t. the introduction of OneShotEvent and not closing connections on substream timeouts, if desired.

[twittner]

Why Clone?

[romanb]

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

[twittner]

Where in the type-system is this enforced?

[romanb]

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

[twittner]

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

[romanb]

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.

[twittner]

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>

[tomaka]

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.

[romanb]

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.

[twittner]

I do not quite understand why this InboundUpgrade does not yield a request, but instead uses oneshot channels to emit the request, wait for the response and send it back all in one go. Is it not the responsibility of a ProtocolsHandler to receive the inbound upgrade, i.e. the request and then command the sending back of the response?

[romanb]

This is motivated here. In essence, it makes for a much simpler implementation (I had the obvious one you suggest before) for a variety of reasons:

1. The protocols handler does not need to track pending inbound substreams as well as the subsequent sending of responses, e.g. via another dedicated FuturesUnordered.
2. The protocols handler does not require another command for sending responses back from the behaviour to the handler and thus TResponse: Clone as well.
3. The protocols handler does not need to track timeouts for pending responses to inbound substreams - the InboundUpgrade already has a timeout managed by the Swarm, just like the OutboundUpgrade. Otherwise, without timeouts after the request has "left" the InboundUpgrade, and the handler itself having to keep inbound substreams around until it received a response, a single omission of RequestResponse::send_response would result in a memory leak in the handler if there is no timeout management in the handler for these substreams. Not only that but being forced to call send_response even for one-way protocols is inconvenient from the perspective of the API. All of these problems are avoided by keeping the read-request/send-response I/O inside the InboundUpgrade::apply_upgrade.
4. Lastly, but certainly not most importantly, there is the symmetry: OutboundUpgrade (send request -> read response); InboundUpgrade (read request -> send response).

[twittner]

I do not question the relative simplicity, but would you not agree that this deviates from how ProtocolHandlers are intended to work? If it turns out that they introduce unnecessary complexity, does this not indicate that their role needs to be revisited?

[romanb]

I do not question the relative simplicity, but would you not agree that this deviates from how ProtocolHandlers are intended to work?

I honestly don't know about whether there is a specific way in which they are intended to work and thus how much this deviates. I chose this implementation purely based on its perceived benefits for the implementation of request/response-per-substream protocols in particular. In the general case, I don't think it is "normal" to send application-level requests/responses in Inbound/Outbound upgrades at all, just to negotiate protocols. In that sense I don't even think it is intended behaviour for an InboundUpgrade to even yield a request.

If it turns out that they introduce unnecessary complexity, does this not indicate that their role needs to be revisited?

I agree in general. I think there was actually some loose agreement at some point on trying to remove the notion of a ProtocolsHandler altogether, possibly alongside rethinking the Swarm, but I don't think anyone has seriously made an attempt to do that so far.

[tomaka]

In the general case, I don't think it is "normal" to send application-level requests/responses in Inbound/Outbound upgrades at all, just to negotiate protocols. In that sense I don't even think it is intended behaviour for an InboundUpgrade to even yield a request.

At the time when these traits were created, they were indeed only supposed to negotiate protocols. However the idea of using them for application-level requests/responses is the natural continuation of this. This pattern is already used everywhere, and I think it is indeed a better pattern compared to just negotiating protocols and handling the application-level logic in the ProtocolsHandler. The only problem to me is that the names InboundUpgrade and OutboundUpgrade do not reflect this.

I agree in general. I think there was actually some loose agreement at some point on trying to remove the notion of a ProtocolsHandler altogether, possibly alongside rethinking the Swarm, but I don't think anyone has seriously made an attempt to do that so far.

At the time when libp2p-swarm was designed, there were multiple factors in play:

• We were still on futures 0.1, and async/await weren't available.
• Rust didn't (and still doesn't) support existential types.
• We had no clear idea of where we were going, so we aimed for maximum flexibility.
• Earlier versions of libp2p were often subject to race conditions.

The current design of libp2p-swarm is unfortunately the trade-off between all these.

[romanb]

At the time when these traits were created, they were indeed only supposed to negotiate protocols. However the idea of using them for application-level requests/responses is the natural continuation of this. This pattern is already used everywhere, and I think it is indeed a better pattern compared to just negotiating protocols and handling the application-level logic in the ProtocolsHandler. The only problem to me is that the names InboundUpgrade and OutboundUpgrade do not reflect this.

That is what I thought, thanks for the clarification.

[twittner]

The new crate needs to be added to the Cargo.toml of libp2p.

[tomaka]

I'll review in details later, but looks good overall, thanks! 👍

[tomaka]

In the general case, I don't think it is "normal" to send application-level requests/responses in Inbound/Outbound upgrades at all, just to negotiate protocols. In that sense I don't even think it is intended behaviour for an InboundUpgrade to even yield a request.

At the time when these traits were created, they were indeed only supposed to negotiate protocols. However the idea of using them for application-level requests/responses is the natural continuation of this. This pattern is already used everywhere, and I think it is indeed a better pattern compared to just negotiating protocols and handling the application-level logic in the ProtocolsHandler. The only problem to me is that the names InboundUpgrade and OutboundUpgrade do not reflect this.

I agree in general. I think there was actually some loose agreement at some point on trying to remove the notion of a ProtocolsHandler altogether, possibly alongside rethinking the Swarm, but I don't think anyone has seriously made an attempt to do that so far.

At the time when libp2p-swarm was designed, there were multiple factors in play:

• We were still on futures 0.1, and async/await weren't available.
• Rust didn't (and still doesn't) support existential types.
• We had no clear idea of where we were going, so we aimed for maximum flexibility.
• Earlier versions of libp2p were often subject to race conditions.

The current design of libp2p-swarm is unfortunately the trade-off between all these.

[tomaka]

Suggestion: maybe add a comment indicating that the response might be silently discarded, in case we took too long to build the response or if the remote has now disconnected?

[romanb]

Yes, I updated the commentary in 7bfdff5.

[tomaka]

Other similar suggestion: maybe add a pub fn is_known_stale(&self) -> bool method to ResponseChannel?

In protocols where building the response to a request is CPU-intensive or IO-intensive, it might be a good idea to call is_known_stale() beforehand and discard the ResponseChannel if true is returned.

Additionally, in situations where the program accumulates incoming requests at faster rate than it processes them, it would help to discard ResponseChannels that we know have already timed out. Otherwise, the program would spend most of its time building responses to timed out requests.

[romanb]

Sounds good. I added ResponseChannel::is_open in 7bfdff5.

[romanb]

Since the code in this PR is now using a custom ProtocolsHandler and not the OneShotHandler, I could revert the changes I made to the OneShotHandler w.r.t. the introduction of OneShotEvent and not closing connections on substream timeouts, if desired.

Just to avoid surprises and since these changes are no longer necessary for this PR, I'm inclined to revert the introduction of the OneShotEvent and the associated semantic change w.r.t. substream timeouts before merging. Let me know if there are objections.