examples/: Add file sharing example #2186

mxinden · 2021-08-08T17:20:03Z

Basic file sharing application with peers either providing or locating
and getting files by name.

While obviously showcasing how to build a basic file sharing
application, the actual goal of this example is to show how to
integrate rust-libp2p into a larger application.

Architectural properties

Clean clonable async/await interface ([Client]) to interact with the
network layer.
Single task driving the network layer, no locks required.

Closes #2158

Compared to #2171 this example would showcase the full idea of a remote control (Client) offering simple async methods with the help of futures::channel::oneshot. I would be curious what you think @thomaseizinger.

@elenaf9 What do you think of this example compared to the larger structure introduced in iotaledger/stronghold.rs#210?

Also, I would be especially curious what people, who are not deeply familiar with libp2p-swarm and libp2p-core already, think about this example. Any suggestions from your side? (//CC @whereistejas @r-zig)

Basic file sharing application with peers either providing or locating and getting files by name. While obviously showcasing how to build a basic file sharing application, the actual goal of this example is **to show how to integrate rust-libp2p into a larger application**. Architectural properties - Clean clonable async/await interface ([`Client`]) to interact with the network layer. - Single task driving the network layer, no locks required.

mxinden · 2021-08-08T18:00:12Z

Also mentioning @umgefahren here, as you were involved on #2158. Feedback very much appreciated.

whereistejas · 2021-08-08T19:49:32Z

I think it is a very good example. I'm myself working on a program where I have four peers in a kademlia network and one of those peers will be operating in client mode. I was having trouble getting the swarm to work, without using async_std::block_on. This program helped me out there. I think, we can have more comments in this example code and others, explaining the intricacies which are otherwise not easily apparent. For example, to use the upgrade method on a transport you need to import the Transport structure. I'm sure people with more experience can come up with better points, in this context.

Also, another thing that can be done, is we can compile a list of good sample projects (with links to specific files) where libp2p is used. I found the following sample program, which I really helped me out: https://github.com/zupzup/rust-peer-to-peer-example

elenaf9

I think this showcases the usage of a remote-control & channels for swarm interaction quite well.
But I'd suggest to split the different parts a bit more into their own function each, for a better understanding and readability.

elenaf9 · 2021-08-08T20:14:22Z

examples/file-sharing.rs

+}
+
+#[derive(Debug, StructOpt)]
+enum Command {


Maybe use a different name for this enum, to differ it from the network::Command enum used for sending commands from the Client to the event-loop. Otherwise it may be a bit confusing.
Maybe CliArgument?

elenaf9 · 2021-08-08T20:25:24Z

examples/file-sharing.rs

+        // Build the Swarm, connecting the lower layer transport logic with the
+        // higher layer network behaviour logic.
+        let mut swarm = SwarmBuilder::new(
+            futures::executor::block_on(libp2p::development_transport(id_keys))?,


Why use futures::executor::block_on here instead of async_std::block_on?
I found it quite hard as a Rust beginner to understand when to use the executor in futures.rs to spawn task and block, and when to use runtimes like tokio or async-std. Projects/ examples in which both are used always confuse(d) me a bit.
Since you are using async-std to spawn the event loop, I'd suggest to either use async-std here as well, or not use it at all and instead LocalPool / ThreadPool from futures::executor to spawn tasks?

elenaf9 · 2021-08-08T20:30:39Z

examples/file-sharing.rs

+
+        loop {
+            futures::select! {
+                event = swarm.next() => match event.expect("Swarm stream to be infinite.") {


Why not use swarm.select_next_some()?

I don't find select_next_some intuitive and usually find myself going back to the method's documentation to make sure I understand all its implications. While a bit more verbose, users should already be familiar with next and expect. I don't feel strongly about it. Please do insist in case you do.

I would agree with going for futures::select! and swarm.next(). This syntax is clearer and easier to understand, for someone who is not familiar with futures.

In this particular case I consider it just to be syntactic sugar, which I mostly like very much given libp2p's verbosity :-)

elenaf9 · 2021-08-08T20:33:57Z

examples/file-sharing.rs

+                    SwarmEvent::UnreachableAddr { .. } => {}
+                    e => panic!("{:?}", e),
+                },
+                command = command_receiver.next() => match command {


I think it would be cleaner to move the two match blocks (here and in line 354) into their own function each.
The event_loop fn is a quite long and thus difficult to understand. Splitting it into smaller functions would help readability and also allow to add some docs on each function.

I am in favor of splitting the large select!, though moving it to separate functions would require passing the pending_* along with each call. With that in mind, I am not sure it would be an improvement.

Or one could use a custom struct wrapping all state providing custom functions to interact with said state. This might also yield the opportunity to DRY.

Thanks for the input @elenaf9 and @wngr. e05117d splits the select! into separate async fns.

elenaf9 · 2021-08-08T20:34:55Z

examples/file-sharing.rs

+                        }
+                    }
+                    SwarmEvent::ConnectionClosed { .. } => {}
+                    SwarmEvent::UnreachableAddr { .. } => {}


Wouldn't we want to return an error for an existing outstanding_dial in this case?

elenaf9 · 2021-08-08T20:37:02Z

examples/file-sharing.rs

+                    )) => {
+                        let sender: oneshot::Sender<()> = outstanding_start_providing
+                            .remove(&id)
+                            .expect("Completed query to be previously outstanding.");


Why use .expect here, and in other cases (e.g. line 379) if let Some(sender) = outstanding_.remove(&id)?

elenaf9 · 2021-08-08T20:52:17Z

examples/file-sharing.rs

+    }
+
+    #[derive(Debug)]
+    enum Command {


Since this is an example that others may use as reference for their own implementation, I would also handle the case here that the operations can fail, hence return a Result in each oneshot::Sender

Command::Dial: Can fail in case of an unreachable address

change sender to sender: oneshot::Sender<Result<(), PendingConnectionError>>

case SwarmEvent::ConnectionEstablished return Ok(())

case SwarmEvent::UnreachableAddr {error, attempts_remaining: 0, ..} return Err(error)

Command::RequestFile: use oneshot::Sender<Result<String, OutboundFailure>>

Command::RespondFile: add sender: oneshot::Sender<Result<(), InboundFailure>>

handle potential error from RequestResponse::send_response

match for SwarmEvent::ResponseSent and SwarmEvent::InboundFailure

... same for StartProviding and GetProviders

The methods in Client would also have to be changed accordingly to return Results,

Agree with changing the signature to be fallible. I would suggest to refrain from returning Swarm errors directly though. If you ever want to implement things like retries, having that in the eventloop directly will result in a less chatty interface between Client and eventloop. A more high-level interface will likely have its own error type hence my suggestion to not use the error from the events directly.

You may want to go even one step further and remove Command::Dial and make it implicit as part of executing other commands. That would highlight that the eventloop is not just a "dumb" loop that forwards everything it gets to the client. (If we are on the same page here but in my experience, you want to push as much "networking" logic as possible down into the eventloop).

elenaf9 · 2021-08-08T20:59:46Z

examples/file-sharing.rs

+//!
+//! ## Sample plot
+//!
+//! Assuming there are 3 nodes, provider A and B and client C.


Using the name "client" here may cause some wrong assumptions regarding the structure network::Client below that is used as Client for the swarm. Maybe use a different wording here.

elenaf9 · 2021-08-08T21:22:46Z

examples/file-sharing.rs

+
+        // In case a listen address was provided use it, otherwise listen on any
+        // address.
+        match listen_address {


I'd do this via an new method Client::start_listening & Command::StartListening, to be consistent.

elenaf9 · 2021-08-08T21:31:15Z

examples/file-sharing.rs

+    /// - The network event stream, e.g. for incoming requests.
+    ///
+    /// - The network task driving the network itself.
+    pub fn new(


This function does quit a lot. I'd split it up into to functions: one for creating the transport, network-behaviour,swarm, and then one for creating the Client and channels related to it. Maybe also move the spawning of the event_loop into that second function. This would highlight this step, and the logic related to it a bit more

thomaseizinger

Overall good, I does showcase the client aspect a bit better. On the other hand, I think it lacks some emphasis that the eventloop should not be dumb but can actually perform significant logic involving its own state and incoming events.

Also added some nits throughout where I think we could write more idiomatic Rust code. I don't think examples should sacrifice on that :)

thomaseizinger · 2021-08-08T22:03:40Z

examples/file-sharing.rs

+    // Creates the network components, namely:
+    //
+    // - The network client to interact with the network layer from anywhere
+    //   within your application.
+    //
+    // - The network event stream, e.g. for incoming requests.
+    //
+    // - The network task driving the network itself.


This is basically repeating the rustdoc, not sure if that is necessary?

thomaseizinger · 2021-08-08T22:04:22Z

examples/file-sharing.rs

+use structopt::StructOpt;
+
+#[async_std::main]
+async fn main() -> Result<(), Box<dyn Error>> {


Why not use anyhow?

thomaseizinger · 2021-08-08T22:05:10Z

examples/file-sharing.rs

+                    // Reply with the content of the file on incoming requests.
+                    Some(network::Event::InboundRequest { request, channel }) => {
+                        if request == name {
+                            let file_content = std::fs::read_to_string(&path)?;


Use async fs module instead?

Why? First I wouldn't expect an async version of read_to_string to lead to any performance gain, given that the actual context switching likely makes up only a small percentage of the overhead involved and second I don't think performance matters in the first place in this case.

That depends a lot on how big the file is, no?

But in any case, the main argument I'd bring forward is that IMO examples shouldn't contain shortcuts that can present footguns but rather should show idiomatic code that can be taken and extended.

Using a sync fs API within an async fn can go wrong if the constraints or requirements around it change.

I don't feel super strongly about it though, so feel free to leave as is! :)

thomaseizinger · 2021-08-08T22:06:40Z

examples/file-sharing.rs

+            // Await the requests, ignore the remaining once a single one succeeds.
+            let file = futures::future::select_ok(requests)
+                .await
+                .map_err(|_| Into::<Box<dyn Error>>::into("None of the providers returned file."))?


With anyhow, this would be a simple .context.

It is just the traide-off between better error handling and yet another dependency increasing compile times / CI times. Given that the main purpose of this example is not showcasing great error handling, I went without anyhow. I cleaned up the map_err a bit now though.

thomaseizinger · 2021-08-08T22:07:00Z

examples/file-sharing.rs

+}
+
+#[derive(Debug, StructOpt)]
+#[structopt(name = "libp2p relay")]


Copy-paste left over? :)

thomaseizinger · 2021-08-08T22:09:51Z

examples/file-sharing.rs

+    pub fn new(
+        listen_address: Option<Multiaddr>,
+        secret_key_seed: Option<u8>,
+    ) -> Result<(Client, BoxStream<'static, Event>, BoxFuture<'static, ()>), Box<dyn Error>> {


This could use impl Trait.

Good catch. Thanks!

thomaseizinger · 2021-08-08T22:10:23Z

examples/file-sharing.rs

+                .unwrap();
+            receiver.await.unwrap();


Document why we can unwrap these?

thomaseizinger · 2021-08-08T22:13:07Z

examples/file-sharing.rs

+        let mut outstanding_dial: HashMap<_, oneshot::Sender<()>> = HashMap::new();
+        let mut outstanding_start_providing = HashMap::new();
+        let mut outstanding_get_providers: HashMap<_, oneshot::Sender<HashSet<PeerId>>> =
+            HashMap::new();
+        let mut outstanding_request_file: HashMap<_, oneshot::Sender<String>> = HashMap::new();


The more common use of outstanding is to express that something is truly excellent. It can mean pending as well but I would probably just resort to pending to be unambiguous :)

elenaf9 · 2021-08-08T22:48:19Z

On the other hand, I think it lacks some emphasis that the eventloop should not be dumb but can actually perform significant logic involving its own state and incoming events.

@thomaseizinger I have a different view on this. In my opinion, the sole responsibility of the event loop should be to just "dumbly" call the Swarm methods according to the Commands it receives, return the result for each prior command once they are ready, and forward incoming events.
I would consider the Client to be somewhat of a Proxy/ Stub of the actual Swarm (with the enhancement that result dont need to be polled, and are returned directly). So any relevant operation that can be performed on the actual swarm, should also be possible via the Client, and the event loop should only forward things.

thomaseizinger · 2021-08-09T00:00:34Z

On the other hand, I think it lacks some emphasis that the eventloop should not be dumb but can actually perform significant logic involving its own state and incoming events.

@thomaseizinger I have a different view on this. In my opinion, the sole responsibility of the event loop should be to just "dumbly" call the Swarm methods according to the Commands it receives, return the result for each prior command once they are ready, and forward incoming events.
I would consider the Client to be somewhat of a Proxy/ Stub of the actual Swarm (with the enhancement that result dont need to be polled, and are returned directly). So any relevant operation that can be performed on the actual swarm, should also be possible via the Client, and the event loop should only forward things.

That is fair! The reason I'd advocate against it is because it results in more code. Every interaction between client and eventloop requires a channel.

If the client acts as a proxy, it doesn't have any more knowledge about what is happening than the swarm. As a result, it should be possible to make every decision that you can make in the client also in the swarm but with less code needing to be written.

To illustrate what I mean, have a look at this snippet of code:

https://github.com/thomaseizinger/rust-libp2p/blob/cb0b5a7177ba2432b1f9b9c3fe0d78c21e2f3082/examples/chat-async.rs#L111-L122

If we would forward every mDNS event, the client would need to react to these generated events, only to tell the event loop to do something it could have achieved autonomously.

Given the controversy of this topic though, I am gonna retract my statement the we need to advocate for one or the other in the example :)

elenaf9 · 2021-08-09T08:43:56Z

The reason I'd advocate against it is because it results in more code. Every interaction between client and eventloop requires a channel.
If the client acts as a proxy, it doesn't have any more knowledge about what is happening than the swarm. As a result, it should be possible to make every decision that you can make in the client also in the swarm but with less code needing to be written.
To illustrate what I mean, have a look at this snippet of code:
https://github.com/thomaseizinger/rust-libp2p/blob/cb0b5a7177ba2432b1f9b9c3fe0d78c21e2f3082/examples/chat-async.rs#L111-L122
If we would forward every mDNS event, the client would need to react to these generated events, only to tell the event loop to do something it could have achieved autonomously.

Good point, I agree that in some cases it makes sense to have more logic in the event loop, to reduce code and overhead.

Another thought:
If this is a lot of logic, I would consider to implement it even one level lower as part of the NetworkBehaviour impl. Either with a custom poll method #[behaviour(poll_method = "poll")], or by manually implementing the whole NetworkBehaviour trait to combine multiple protocols (like the proc_macro does it), and then add some logic on top.

But I agree that this very much depends on the concrete application and use-case.

mxinden · 2021-08-16T15:58:19Z

I very much appreciate all the input @thomaseizinger, @elenaf9, @wngr and @whereistejas! Great to see the many perspectives.

I addressed most but not all of your comments. I will merge this already as I find it to be in a useful state today. Follow-up improvements are always welcome.

Hope this pull request makes getting-started with rust-libp2p a little easier.

* protocols/gossipsub: Fix inconsistency in mesh peer tracking (libp2p#2189) Co-authored-by: Age Manning <Age@AgeManning.com> * misc/metrics: Add auxiliary crate to record events as OpenMetrics (libp2p#2063) This commit adds an auxiliary crate recording protocol and Swarm events and exposing them as metrics in the OpenMetrics format. * README: Mention security@ipfs.io * examples/: Add file sharing example (libp2p#2186) Basic file sharing application with peers either providing or locating and getting files by name. While obviously showcasing how to build a basic file sharing application, the actual goal of this example is **to show how to integrate rust-libp2p into a larger application**. Architectural properties - Clean clonable async/await interface ([`Client`]) to interact with the network layer. - Single task driving the network layer, no locks required. * examples/README: Give an overview over the many examples (libp2p#2194) * protocols/kad: Enable filtering of (provider) records (libp2p#2163) Introduce `KademliaStoreInserts` option, which allows to filter records. Co-authored-by: Max Inden <mail@max-inden.de> * swarm/src/protocols_handler: Impl ProtocolsHandler on either::Either (libp2p#2192) Implement ProtocolsHandler on either::Either representing either of two ProtocolsHandler implementations. Co-authored-by: Thomas Eizinger <thomas@eizinger.io> * *: Make libp2p-core default features optional (libp2p#2181) Co-authored-by: Max Inden <mail@max-inden.de> * core/: Remove DisconnectedPeer::set_connected and Pool::add (libp2p#2195) This logic seems to be a leftover of libp2p#889 and unused today. * protocols/gossipsub: Use ed25519 in tests (libp2p#2197) With f2905c0 the secp256k1 feature is disabled by default. Instead of enabling it in the dev-dependency, simply use ed25519. * build(deps): Update minicbor requirement from 0.10 to 0.11 (libp2p#2200) Updates the requirements on [minicbor](https://gitlab.com/twittner/minicbor) to permit the latest version. - [Release notes](https://gitlab.com/twittner/minicbor/tags) - [Changelog](https://gitlab.com/twittner/minicbor/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/twittner/minicbor/compare/minicbor-v0.10.0...minicbor-v0.11.0) --- updated-dependencies: - dependency-name: minicbor dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): Update salsa20 requirement from 0.8 to 0.9 (libp2p#2206) * build(deps): Update salsa20 requirement from 0.8 to 0.9 Updates the requirements on [salsa20](https://github.com/RustCrypto/stream-ciphers) to permit the latest version. - [Release notes](https://github.com/RustCrypto/stream-ciphers/releases) - [Commits](RustCrypto/stream-ciphers@ctr-v0.8.0...salsa20-v0.9.0) --- updated-dependencies: - dependency-name: salsa20 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * *: Bump pnet to v0.22 Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Max Inden <mail@max-inden.de> * *: Dial with handler and return handler on error and closed (libp2p#2191) Require `NetworkBehaviourAction::{DialPeer,DialAddress}` to contain a `ProtocolsHandler`. This allows a behaviour to attach custom state to its handler. The behaviour would no longer need to track this state separately during connection establishment, thus reducing state required in a behaviour. E.g. in the case of `libp2p-kad` the behaviour can include a `GetRecord` request in its handler, or e.g. in the case of `libp2p-request-response` the behaviour can include the first request in the handler. Return `ProtocolsHandler` on connection error and close. This allows a behaviour to extract its custom state previously included in the handler on connection failure and connection closing. E.g. in the case of `libp2p-kad` the behaviour could extract the attached `GetRecord` from the handler of the failed connection and then start another connection attempt with a new handler with the same `GetRecord` or bubble up an error to the user. Co-authored-by: Thomas Eizinger <thomas@eizinger.io> * core/: Remove deprecated read/write functions (libp2p#2213) Co-authored-by: Max Inden <mail@max-inden.de> * protocols/ping: Revise naming of symbols (libp2p#2215) Co-authored-by: Max Inden <mail@max-inden.de> * protocols/rendezvous: Implement protocol (libp2p#2107) Implement the libp2p rendezvous protocol. > A lightweight mechanism for generalized peer discovery. It can be used for bootstrap purposes, real time peer discovery, application specific routing, and so on. Co-authored-by: rishflab <rishflab@hotmail.com> Co-authored-by: Daniel Karzel <daniel@comit.network> * core/src/network/event.rs: Fix typo (libp2p#2218) * protocols/mdns: Do not fire all timers at the same time. (libp2p#2212) Co-authored-by: Max Inden <mail@max-inden.de> * misc/metrics/src/kad: Set query_duration lowest bucket to 0.1 sec (libp2p#2219) Probability for a Kademlia query to return in less than 100 milliseconds is low, thus increasing the lower bucket to improve accuracy within the higher ranges. * misc/metrics/src/swarm: Expose role on connections_closed (libp2p#2220) Expose whether closed connection was a Dialer or Listener. * .github/workflows/ci.yml: Use clang 11 (libp2p#2233) * protocols/rendezvous: Update prost (libp2p#2226) Co-authored-by: Max Inden <mail@max-inden.de> * *: Fix clippy warnings (libp2p#2227) * swarm-derive/: Make event_process = false the default (libp2p#2214) Co-authored-by: Max Inden <mail@max-inden.de> Co-authored-by: Max Inden <mail@max-inden.de> Co-authored-by: Age Manning <Age@AgeManning.com> Co-authored-by: Ruben De Smet <ruben.de.smet@rubdos.be> Co-authored-by: Thomas Eizinger <thomas@eizinger.io> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: rishflab <rishflab@hotmail.com> Co-authored-by: Daniel Karzel <daniel@comit.network> Co-authored-by: David Craven <david@craven.ch>

elenaf9 reviewed Aug 8, 2021

View reviewed changes

thomaseizinger approved these changes Aug 8, 2021

View reviewed changes

mxinden added 13 commits August 11, 2021 12:23

examples/file-sharing: Rename Command to CliArgument

f875bfd

examples/file-sharing: Don't block_on dns transport but await instead

09f28ce

examples/file-sharing: Propagate dial failure

6e9c9f2

examples/file-sharing: Expect pending outbound provider query

99c28f3

examples/file-sharing: Use str as error

7521919

examples/file-sharing: Fix structopt title

aa483c8

examples/file-sharing: Use impl Future and impl Stream

3fba99a

examples/file-sharing: Rename outstanding to pending

eaf81f5

examples/file-sharing: Showcase fallible Client methods

13a7d02

examples/file-sharing: Do not overload term Client

7d76961

examples/file-sharing: Trigger listening via client

8ca3c2a

examples/file-sharing: Remove duplicate comment

fe5cba4

examples/file-sharing: Replace most unwrap calls

64b8a68

thomaseizinger mentioned this pull request Aug 12, 2021

Make event_process = false the default on derive(NetworkBehaviour) #2190

Closed

elenaf9 approved these changes Aug 12, 2021

View reviewed changes

mxinden added 2 commits August 16, 2021 16:45

examples/file-sharing: Wrap state in EventLoop struct

e05117d

Merge branch 'master' into example-file-sharing

ed2c20c

mxinden merged commit d3f5a1e into libp2p:master Aug 16, 2021

mxinden mentioned this pull request Aug 23, 2021

Add async chat example #2171

Closed

whereistejas mentioned this pull request Sep 20, 2021

Implement K-DHT for spartan-farmer autonomys/subspace#27

Closed

5 tasks

examples/: Add file sharing example #2186

examples/: Add file sharing example #2186

Conversation

mxinden commented Aug 8, 2021

mxinden commented Aug 8, 2021

whereistejas commented Aug 8, 2021

elenaf9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 commented Aug 8, 2021 • edited Loading

thomaseizinger commented Aug 9, 2021

elenaf9 commented Aug 9, 2021

mxinden commented Aug 16, 2021

elenaf9 Aug 8, 2021 •

edited

Loading

mxinden Aug 11, 2021 •

edited

Loading

elenaf9 commented Aug 8, 2021 •

edited

Loading