feat: allow selecting outbound protocols in request_response behaviour

[MegaRedHand]

Description

The current request_response module handles incoming requests for any of the configured protocols, negotiating the protocol on each request, but can only send requests for all of the configured protocols. This makes it impossible to handle multiple request types with a single codec.

Problem example

My req-resp server supports two protocols A and B.

If a peer sends a request for protocol A, my server sends a response with protocol A (i.e. the codec receives A as parameter). If a peer sends a request for protocol B, my server sends a response with protocol B.

If I send a request through send_request, however, the Behaviour automatically fills the protocols as [A, B], which seems to resolve to A always during negotiation. Using the new send_request_with_protocol method, however, let's me specify B as the protocol to request for.

Notes & open questions

• send_request_with_protocol receives a single protocol. This could be extended to receive an iterator of protocols.
• The new function repeats code with send_request_with_addresses. The repeated code could be moved to a new function receiving both addresses and protocols.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[drHuangMHT]

If the protocol you are talking about is a StreamProtocol, which is essentially a &str, it is not possible to support two protocols in the same request_response::Behaviour, because StreamProtocol is strictly one-to-one. The receiving end must support the exact same StreamProtocol to open substreams for that protocol. For protocol selection, it should involve ConnectionHandlers.
I don't think you are using request-response correctly, because you can have multiple request_response::Behaviour instances in your swarm, as long as they use different StreamProtocol. But you seem to be assigning multiple StreamProtocols to one single Behaviour.
actually I read the docs again, and I found:

rust-libp2p/protocols/request-response/src/lib.rs

Lines 50 to 55 in b2a9000

	//! ## Protocol Families
	//!
	//! A single [`Behaviour`] instance can be used with an entire
	//! protocol family that share the same request and response types.
	//! For that purpose, [`Codec::Protocol`] is typically
	//! instantiated with a sum type.

So as long as the type for request and response are the same, it's OK to support multiple StreamProtocols in one request-response::Behaviour.

[drHuangMHT]

Maybe we should check against outbound_protocol on the behaviour first? protocols outside of outbound_protocol may be negotiated successfully, but I don't think it is a correct behaviour.

[MegaRedHand]

I agree. Allowing that will probably just lead to confusion

[drHuangMHT]

What if a user want to send a request of a specific protocol, and a specific address, given that we also have send_request_with_addresses?

[MegaRedHand]

That request isn't supported by this interface.

I was thinking about adding a new function receiving an options argument (similar interface as DialOpts). That'd replace this function and simplify/deprecate the addresses one.

[drHuangMHT]

request_response::Message does not carry information about the protocol through which the request is sent, in other words, requests seen on the receiving end are outbound-protocol-agnostic.

[MegaRedHand]

request_response::Message does not carry information about the protocol through which the request is sent, in other words, requests seen on the receiving end are outbound-protocol-agnostic.

I don't think that's a problem. The Codec interface doesn't support multiple request or response types unless they are inside an enum, which would remove the agnosticity of the receiving end anyways.

In our case, we have an enum for requests and another for responses, which implicitly define the protocol we're using. The Codec has access to the negotiated protocol, so it knows which enum variant to read from the wire on the receiving side, and then wraps it in the enum, sending it to the application. Relevant code

[drHuangMHT]

I don't think that's a problem. The Codec interface doesn't support multiple request or response types unless they are inside an enum, which would remove the agnosticity of the receiving end anyways.

If TRequest(and TResponse respectively) carry the protocol itself, which is what you do, it does inherently make Message protocol-aware. So I assume you are reusing enum variant for request(and/or response) for different endpoints, which means without explicitly including protocol information in the variant there can be a mismatch?
EDIT: No, the awareness comes from custom implementation of Codec, and the stream request.

In our case, we have an enum for requests and another for responses, which implicitly define the protocol we're using.

I think implementing serializing and deserializing on the enum instead of its variants works better, which makes Message protocol-agnostic, one endpoint one variant. And upon receiving the request enum, delegate the inner request to the respective endpoint.

The Codec has access to the negotiated protocol, so it knows which enum variant to read from the wire on the receiving side, and then wraps it in the enum, sending it to the application.

Codec trait does have access to Protocol, but the provided implementation json and cbor ignore the argument. I assume you also have your own implementation of Codec?

I read your code. I've been assuming the simplest implementation, but you are implementing Codec yourself to receive protocols passed to SubstreamRequest as defined here:

rust-libp2p/protocols/request-response/src/handler.rs

Lines 450 to 452 in b2a9000

	return Poll::Ready(ConnectionHandlerEvent::OutboundSubstreamRequest {
	protocol: SubstreamProtocol::new(Protocol { protocols }, ()),
	});

So it is a missing feature to be able to select which protocol to use. I think to simplify the interface we should work on send_request itself to make it accept which protocols to use. thoughts @elenaf9 ?