Add a send_request function to NetworkService

[eskimor]

This function differs from the already provided request method in two ways.

1. It allows sending of messages to currently disconnected peers, which is required for my first go at request based availability distribution.
2. Instead of creating a sender/receiver pair and awaiting the receiver, it takes a sender as argument. This suits our use in Polkadot better, where the subsystem will hold the corresponding receiver.

This PR changes the signature of some apparently only internally used send_request functions to accept an additional boolean parameter.

[bkchr]

2\. Instead of creating a sender/receiver pair and awaiting the receiver, it takes a sender as argument. This suits our use in Polkadot better, where the subsystem will hold the corresponding receiver.

The request function is returning a Future. So, you have exactly the same interface as with your oneshot::Receiver. You have a future that you can await on.

[eskimor]

The request function is returning a Future. So, you have exactly the same interface as with your oneshot::Receiver. You have a future that you can await on.

That's quite right: For response - I get a future, which I would have to thread back to the sending subsystem. For send_request that's not necessary, as the sending subsystem already has the corresponding future.

Assuming the end user of the API would be the function caller this would of course be correct, but that is not the case in Polkadot, where subsystems communicate with the network bridge via message passing.

[bkchr]

Assuming the end user of the API would be the function caller this would of course be correct, but that is not the case in Polkadot, where subsystems communicate with the network bridge via message passing.

The polkadot subsystem can just send its oneshot::Sender and you do async move { sender.send(future.await); }.

[eskimor]

The polkadot subsystem can just send its oneshot::Sender and you do async move { sender.send(future.await); }

That's true, but it felt stupid to create another sender/receiver pair when there is one already.

Like the API got changed the very last moment from something I wanted to something else and then I change it right back again... Felt better to just expose what I want.

[tomaka]

I'd strongly prefer to have the same API as request:

• Adjusting the API in Substrate to the implementation details of Polkadot feels wrong.
• Passing a sender also makes it impossible to implement cancelling requests if the future is dropped.
• The receiving side of a channel can produce a "cancelled" error if the sender has been closed, which needs to be handled properly and is less explicit than RequestFailure.

I would encourage you to use a Pin<Box<dyn Future<Output = ...>>> in Polkadot to inform of the response, instead of a Receiver<...>.

[eskimor]

Hmm, not sure I can agree. The request code path is already quite hard to follow, with quite a few layers of indirection. Not exposing the API that is already there and is needed/preferred in Polkadot, just adds another layer. Given the fact that it is implemented internally as a sender/receiver pair also suggests that it is not an esoteric use case in Polkadot to prefer such an API, but rather something that is likely to come up often (every time someone uses message passing), as we already depend on such an API both in Substrate and in Polkadot, for good reasons.

* Passing a sender also makes it impossible to implement cancelling requests if the future is dropped.

How? There is poll_canceled to do precisely that. At least if I understood your concern correctly.

* The receiving side of a channel can produce a "cancelled" error if the sender has been closed, which needs to be handled properly and is less explicit than `RequestFailure`.

That is true, but obviously already handled on the Polkadot side. I agree, that this is less explicit than a constructor in RequestFailure, but on the other hand getting a cancelled error, it is still pretty clear what happened.

[eskimor]

Expanding on the reasoning, I would see the request function as a good and easy default for simple use cases and the send_request function as a more flexible/more bare-bone interface for more advanced use cases. I also did not get the feeling, send_request would expose some rather hidden implementation detail or something, although I am sensing you are suggesting something in that direction? What are the reasons, you think exposing a Sender/Receiver interface is not such a good idea? Using a <dyn Future> is sure more generic, but I am not sure why we would want/need that level of generality here. In my view a oneshot::Sender/Receiver is a pretty good standard infra to build upon.

[eskimor]

I would encourage you to use a Pin<Box<dyn Future<Output = ...>>> in Polkadot to inform of the response, instead of a Receiver<...>.

If that is about hiding implementation details and getting the nicer API, that all errors are contained in RequestFailure, wrapping the Receiver in a IncomingRequestReceiver struct might even be nicer and also do the trick? Although I am not sure, wrapping the oneshot::Receiver API just for that is worth it.

[tomaka]

Yes my grief is about exposing implementation details. I'd rather not tie the networking API to channels.
At the moment, there is one, for receiving incoming requests, not because we're using a channel internally (we're not) but because I felt that it would be easy from a user perspective to understand an API where you plug a channel sender, while any alternative would have been way more complicated.
There's no complexity problem for request. It's simple to use.

but I am not sure why we would want/need that level of generality here

What's wrong with Pin<Box<dyn Future>>? What is the reason why we wouldn't want it?

[eskimor]

Yes my grief is about exposing implementation details. I'd rather not tie the networking API to channels.
At the moment, there is one, for receiving incoming requests, not because we're using a channel internally (we're not) but because I felt that it would be easy from a user perspective to understand an API where you plug a channel sender, while any alternative would have been way more complicated.

I am not sure we should be seeing the channels as an implementation detail. As you mentioned, we already have a channel based interface for receiving incoming requests. It makes sense to me to offer such an interface also for sending requests (at least optionally, as suggested with send_request). It is also not an uncommon interface for a future based API, is quite nice for message passing and also it is already there, we are just hiding it.

Do we have plans on getting rid of the channel internally? Exposing a channel based API even in that case, would not be an issue, but I agree that if that is the case we should have a solid case of exposing such an API to be a good user interface, as then we would end up just making it up for the user facing code.

There's no complexity problem for request. It's simple to use.

For simple use cases, but not as nice for communication via message passing which is not uncommon in multithreaded environments (Polkadot and Substrate being good examples). It is of course not a terrible issue to create a channel based interface on top, but I would also rather not needlessly complicate things more.

What's wrong with Pin<Box<dyn Future>>? What is the reason why we wouldn't want it?

It feels a bit clunky and needlessly dynamic, in particular I am not sure I understand why I should prefer it over a Receiver. But I guess, that boils down to whether we want channels in the public API or not.

* Adjusting the API in Substrate to the implementation details of Polkadot feels wrong.

I am also not sure about this. If Polkadot was somehow esoteric with patterns no-one other should ever use, then yes. But if it is just a user of our API, then its needs should absolutely inform our API design. Of course we should also not delve into featuritis, that would not help either :-)

[tomaka]

I looked at your Polkadot PR, and I now realize that you're creating the channel ahead of time in a total different part of the code.
I still don't think this API makes sense when taken in isolation, but I can get behind the argument that "changing Polkadot would require either a big refactoring or introducing code that's a little stupid".

I'll do another review.

Explained above

[tomaka]

I think it would make sense if the only difference between request and send_request/other-name is that the latter sends the response on the channel.

[eskimor]

Ok, I introduced the IfDisconnectedBehaviour and updated the docs. I ended up introducing IfDisconnectedBehaviour also for the internal function calls as it made the code more readable and straight forward.

I did shorten the name though from IfDisconnectedBehaviour to just IfDisconnected. Reason being, I introduced that enum right between other Behaviours, which are behaviours in the libp2p sense, so that name for the enum seemed slightly confusing. Also the shorter name makes function calls a bit more readable as well: IfDisconnected::TryConnect ... reads almost like an English sentence describing what happens.

request now also takes that additional parameter.

[romanb]

Generally looks ok to me, I just left some small suggestions for naming and API comments that you can choose to ignore if you don't consider them an improvement. However, if I understand correctly, this relies on the automatic dialing of libp2p-request-response. Note that substrate's RequestResponsesBehaviour still considers it an error if an inner RequestResponseBehaviour emits a DialPeer command. This error logging should probably be removed then to avoid confusion.

[eskimor]

Thanks a lot @romanb! Lots of good catches!

[eskimor]

Why does the github-actions keep adding 'A7-needspolkadotpr`, when this is clearly not the case? Because I referenced this PR from a Polkadot PR?