New and improved qp2p API #206

lionel-faber · 2021-02-01T11:34:50Z

This PR introduces a new and improved simple qp2p API. This also implements connection and message handling within the qp2p crate itself so that the user of qp2p does not need to manage them.

madadam · 2021-02-01T12:26:03Z

src/endpoint.rs

+    /// Sends a message to a peer. This will attempt to re-use any existing connections
+    /// with the said peer. If a connection doesn't exist already, a new connection will be created.
+    pub async fn send_message(&self, msg: Bytes, dest: &SocketAddr) -> Result<()> {
+        self.connect_to(dest).await?;


Why not return the connection directly from connect_to? Then we wouldn't have to look it up again using get_connection here.

The connection object is not a part of the public API. It is held internally and handled within this crate.

madadam · 2021-02-01T12:26:16Z

src/endpoint.rs

+    /// Returns a message received from another peer along with the source address
+    pub async fn next_incoming_message(&mut self) -> Option<(SocketAddr, Bytes)> {
+        self.message_queue.lock().await.pop_front()
+    }


First of all, you don't need &mut self because the message queue is behind a Mutex. But more serious issue is that this seems like it forces us to busy-loop for incoming messages, wasting CPU cycles. Wonder why do we need the message queue at all. Why not just recv on the message channel receiver instead? This same point applies to next_incoming_connection and next_disconnected_peer too.

madadam · 2021-02-01T12:31:10Z

src/connections.rs

+            };
+            if let Some(message) = message {
+                message_tx
+                    .send((src, message.get_message_data()))


Why even create the Message enum when only the data is used here?

Yes. I'm going to remove the Message enum completely.

madadam · 2021-02-01T12:37:23Z

Cargo.toml

@@ -21,6 +21,7 @@ serde_json = "1.0.59"
 structopt = "~0.3.15"
 thiserror = "1.0.23"
 webpki = "~0.21.3"
+flexi_logger = "~0.16.1"


What's the reason of moving this from dev-dependencies to here?

madadam · 2021-02-01T12:45:59Z

src/endpoint.rs

+    pub fn disconnect_from(&mut self, peer_addr: &SocketAddr) -> Result<()> {
+        if let Some((conn, remover)) = self.connection_pool.remove(peer_addr) {
+            conn.close(0u8.into(), b"User Disconnected");
+            remover.remove();


You already removed the connection from the pool by calling ConnectionPool::remove above, so there is no need to remove it via the remover too.

madadam · 2021-02-01T12:47:25Z

src/connection_pool.rs

+            .is_some()
+    }
+
+    pub fn remove(&mut self, addr: &SocketAddr) -> Option<(quinn::Connection, ConnectionRemover)> {


There is no point in returning the Remover here. The remover is a handle to remove the connection from the pool. But you are doing that manually in this function, so the remover has no purpose (calling remove on it would have no effect).

Also, this removes only one connection with that address, but in some cases (*) there can be more than one. Wonder if it wouldn't make more sense to remove all of them instead?

*) if two peers open a connection to each other around the same time, they can end up having two connections to the same peer in the pool (one incoming and one outgoing).

bochaco · 2021-02-01T13:22:17Z

src/endpoint.rs

        } else {
-            Err(Error::NoEchoServiceResponse)
+            log::info!("We are not connected to this peer. Doing nothing");


Perhaps the message should be more informative about what it was attempting to do and that it's not doing it since there is not connection, otherwise when reading the logs you cannot understand what is it that "it's not doing".

Of course :) I was saving log clean-up for the end 😄

Do not bind ever to 0.0.0.0

this can be done by saving the result of the echo service after calling it the first time BREAKING CHANGE

remove chat functionality

arguments

to qp2p this allows us to provide a simple API that can be used to send/receive messages

- split out the recievers out of the endpoint object - fix all tests and doc tests to work with the new API

stream data

this example is no longer relevant since the echo service is automatically handled within the qp2p crate

and port is provided manually

is created - this simplifies the Endpoint::socket_addr() API removing the need of a mutable borrow

successful bootstrap

madadam · 2021-02-11T14:11:42Z

src/endpoint.rs

+    }
+
+    /// Sends a message to a peer. This will attempt to re-use any existing connections
+    /// with the said peer. If a connection doesn't exist already, a new connection will be created.


If a connection doesn't exist already, a new connection will be created

This doesn't seem to match the implementation.

event via the streams - this also prevents the creation of a new connection when send_message is called

… used across threads. * modifies ConnectionDeduplicator to use Arc<Mutex<_>> instead of Mutex<_> * makes ConnectionDeduplicator cloneable

madadam reviewed Feb 1, 2021

View reviewed changes

bochaco reviewed Feb 1, 2021

View reviewed changes

lionel-faber force-pushed the refactor-api branch 4 times, most recently from 973c609 to 330ddd2 Compare February 3, 2021 05:41

lionel-faber marked this pull request as ready for review February 3, 2021 06:08

lionel-faber requested a review from a team as a code owner February 3, 2021 06:08

dirvine and others added 17 commits February 11, 2021 13:09

feature(default ip) Use localhost as IP.

6777e94

Do not bind ever to 0.0.0.0

chore(fmt) cargo fmt

30cacc2

chore(refactor) Simplify some statements.

590c2a4

fix(example): dont use LocalHost in example

3c89560

fix(igd): don't skip port forwarding if local IP address is specified

083be66

fix(echo_service): prevent contacting the echo service multiple times

f2d27b2

this can be done by saving the result of the echo service after calling it the first time BREAKING CHANGE

refactor(example): refactor echo service example to use StructOpt and

fcf1ea2

remove chat functionality

feat(api): add support for manual port forwarding by passing additional

c9acf77

arguments

feat(api): move all connection handling and message handling internally

17a7c61

to qp2p this allows us to provide a simple API that can be used to send/receive messages

feat(disconnects): add API for disconnection and fix tests

aa02c96

fix(test): refactor structure of test code and fix echo_service test

de24107

fix(all): remove FIFO queues and use the mpsc channels directly

4e6e40b

- split out the recievers out of the endpoint object - fix all tests and doc tests to work with the new API

feat(api): add API to open bi-directional stream that can be used to

3cf5aee

stream data

remove(example): remove echo_service example

9b402f1

this example is no longer relevant since the echo service is automatically handled within the qp2p crate

feat(echo_service): find if peer is externally reachable if external IP

7dcba12

and port is provided manually

feat(echo_service): perform UPnP and/or echo_service when the endpoint

631a19e

is created - this simplifies the Endpoint::socket_addr() API removing the need of a mutable borrow

refactor(api): return the socket address of the connected peer after

db83038

successful bootstrap

lionel-faber force-pushed the refactor-api branch from 8d2b17a to 479bb18 Compare February 11, 2021 12:53

madadam reviewed Feb 11, 2021

View reviewed changes

refactor(comms): prevent early exit of control flow when sending an

860ea5b

event via the streams - this also prevents the creation of a new connection when send_message is called

lionel-faber force-pushed the refactor-api branch from 479bb18 to 860ea5b Compare February 11, 2021 16:09

feat: makes QuicP2p::Endpoint cloneable so that it can more easily be…

bd8d21d

… used across threads. * modifies ConnectionDeduplicator to use Arc<Mutex<_>> instead of Mutex<_> * makes ConnectionDeduplicator cloneable

feat: adds a p2p node example that uses bidirectional streams

4e02196

joshuef approved these changes Feb 12, 2021

View reviewed changes

lionel-faber merged commit 46184b8 into maidsafe:master Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New and improved qp2p API #206

New and improved qp2p API #206

lionel-faber commented Feb 1, 2021

madadam Feb 1, 2021

lionel-faber Feb 2, 2021

madadam Feb 1, 2021

madadam Feb 1, 2021

lionel-faber Feb 2, 2021

madadam Feb 1, 2021

madadam Feb 1, 2021

madadam Feb 1, 2021

madadam Feb 1, 2021

bochaco Feb 1, 2021

lionel-faber Feb 1, 2021

madadam Feb 11, 2021

New and improved qp2p API #206

New and improved qp2p API #206

Conversation

lionel-faber commented Feb 1, 2021

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