Add more tests, fix (and test for) a deadlock re overquota messages, more unbounded channels and less .awaiting

backend/telemetry_core/tests/e2e_tests.rs

@@ -1,12 +1,15 @@
 use common::node_types::BlockHash;
+use futures::StreamExt;
 use serde_json::json;
-use std::time::Duration;
+use std::{iter::FromIterator, time::Duration};
 use test_utils::{
     assert_contains_matches,
     feed_message_de::{FeedMessage, NodeDetails},
     workspace::start_server_debug
 };
 
+/// The simplest test we can run; the main benefit of this test (since we check similar)
+/// below) is just to give a feel for _how_ we can test basic feed related things.
 #[tokio::test]
 async fn feed_sent_version_on_connect() {
     let server = start_server_debug().await;
@@ -26,6 +29,8 @@ async fn feed_sent_version_on_connect() {
     server.shutdown().await;
 }
 
+/// Another very simple test: pings from feeds should be responded to by pongs
+/// with the same message content.
 #[tokio::test]
 async fn feed_ping_responded_to_with_pong() {
     let server = start_server_debug().await;
@@ -49,6 +54,109 @@ async fn feed_ping_responded_to_with_pong() {
     server.shutdown().await;
 }
 
+
+/// As a prelude to `lots_of_mute_messages_dont_cause_a_deadlock`, we can check that
+/// a lot of nodes can simultaneously subscribe and are all sent the expected response.
+#[tokio::test]
+async fn multiple_feeds_sent_version_on_connect() {
+    let server = start_server_debug().await;
+
+    // Connect a bunch of feeds:
+    let mut feeds = server
+        .get_core()
+        .connect_multiple(1000)
+        .await
+        .unwrap();
+
+    // Wait for responses all at once:
+    let responses = futures::future::join_all(
+        feeds.iter_mut()
+        .map(|(_, rx)| rx.recv_feed_messages())
+    );
+
+    let responses = tokio::time::timeout(Duration::from_secs(10), responses)
+        .await
+        .expect("we shouldn't hit a timeout waiting for responses");
+
+    // Expect a version response of 31 to all of them:
+    for feed_messages in responses {
+        assert_eq!(
+            feed_messages.expect("should have messages"),
+            vec![FeedMessage::Version(31)],
+            "expecting version"
+        );
+    }
+
+    // Tidy up:
+    server.shutdown().await;
+}
+
+/// When a lot (> ~700 in this case) of nodes are added, the chain becomes overquota.
+/// this leads to a load of messages being sent back to the shard. If bounded channels
+/// are used to send messages back to the shard, it's possible that we get into a situation
+/// where the shard is waiting trying to send the next "add node" message, while the
+/// telemetry core is waiting trying to send up to the shard the next "mute node" message,
+/// resulting in a deadlock. This test gives confidence that we don't run into such a deadlock.
+#[tokio::test]
+async fn lots_of_mute_messages_dont_cause_a_deadlock() {
+    let mut server = start_server_debug().await;
+    let shard_id = server.add_shard().await.unwrap();
+
+    // Connect 1000 nodes to the shard:
+    let mut nodes = server
+        .get_shard(shard_id)
+        .unwrap()
+        .connect_multiple(2000) // 1500 of these will be overquota.
+        .await
+        .expect("nodes can connect");
+
+    // Every node announces itself on the same chain:
+    for (idx, (node_tx, _)) in nodes.iter_mut().enumerate() {
+        node_tx.send_json_text(json!({
+            "id":1, // message ID, not node ID. Can be the same for all.
+            "ts":"2021-07-12T10:37:47.714666+01:00",
+            "payload": {
+                "authority":true,
+                "chain":"Local Testnet",
+                "config":"",
+                "genesis_hash": BlockHash::from_low_u64_ne(1),
+                "implementation":"Substrate Node",
+                "msg":"system.connected",
+                "name": format!("Alice {}", idx),
+                "network_id":"12D3KooWEyoppNCUx8Yx66oV9fJnriXwCcXwDDUA2kj6vnc6iDEp",
+                "startup_time":"1625565542717",
+                "version":"2.0.0-07a1af348-aarch64-macos"
+            }
+        })).await.unwrap();
+    }
+
+    // Wait a little time (just to let everything get deadlocked) before
+    // trying to have the aggregator send out feed messages.
+    tokio::time::sleep(std::time::Duration::from_millis(500)).await;
+
+    // Start a bunch of feeds. If deadlock has happened, none of them will
+    // receive any messages back.
+    let mut feeds = server
+        .get_core()
+        .connect_multiple(1)
+        .await
+        .expect("feeds can connect");
+
+    // Wait to see whether we get anything back:
+    let msgs_fut = futures::future::join_all(
+        feeds
+            .iter_mut()
+            .map(|(_,rx)| rx.recv_feed_messages())
+    );
+
+    // Give up after a timeout:
+    tokio::time::timeout(Duration::from_secs(10), msgs_fut)
+        .await
+        .expect("should not hit timeout waiting for messages (deadlock has happened)");
+}
+
+/// If a node is added, a connecting feed should be told about the new chain.
+/// If the node is removed, the feed should be told that the chain has gone.
 #[tokio::test]
 async fn feed_add_and_remove_node() {
     // Connect server and add shard
@@ -111,6 +219,90 @@ async fn feed_add_and_remove_node() {
     server.shutdown().await;
 }
 
+/// If nodes connect and the chain name changes, feeds will be told about this
+/// and will keep receiving messages about the renamed chain (despite subscribing
+/// to it by name).
+#[tokio::test]
+async fn feeds_told_about_chain_rename_and_stay_subscribed() {
+    // Connect a node:
+    let mut server = start_server_debug().await;
+    let shard_id = server.add_shard().await.unwrap();
+    let (mut node_tx, _node_rx) = server
+        .get_shard(shard_id)
+        .unwrap()
+        .connect()
+        .await
+        .expect("can connect to shard");
+
+    let node_init_msg = |id, chain_name: &str, node_name: &str| json!({
+        "id":id,
+        "ts":"2021-07-12T10:37:47.714666+01:00",
+        "payload": {
+            "authority":true,
+            "chain": chain_name,
+            "config":"",
+            "genesis_hash": BlockHash::from_low_u64_ne(1),
+            "implementation":"Substrate Node",
+            "msg":"system.connected",
+            "name": node_name,
+            "network_id":"12D3KooWEyoppNCUx8Yx66oV9fJnriXwCcXwDDUA2kj6vnc6iDEp",
+            "startup_time":"1625565542717",
+            "version":"2.0.0-07a1af348-aarch64-macos"
+        },
+    });
+
+    // Subscribe a chain:
+    node_tx.send_json_text(node_init_msg(1, "Initial chain name", "Node 1")).await.unwrap();
+
+    // Connect a feed and subscribe to the above chain:
+    let (mut feed_tx, mut feed_rx) = server.get_core().connect().await.unwrap();
+    feed_tx.send_command("subscribe", "Initial chain name").await.unwrap();
+
+    // Feed is told about the chain, and the node on this chain:
+    let feed_messages = feed_rx.recv_feed_messages().await.unwrap();
+    assert_contains_matches!(
+        feed_messages,
+        FeedMessage::AddedChain { name, node_count: 1 } if name == "Initial chain name",
+        FeedMessage::SubscribedTo { name } if name == "Initial chain name",
+        FeedMessage::AddedNode { node: NodeDetails { name: node_name, .. }, ..} if node_name == "Node 1",
+    );
+
+    // Subscribe another node. The chain doesn't rename yet but we are told about the new node
+    // count and the node that's been added.
+    node_tx.send_json_text(node_init_msg(2, "New chain name", "Node 2")).await.unwrap();
+    let feed_messages = feed_rx.recv_feed_messages().await.unwrap();
+    assert_contains_matches!(
+        feed_messages,
+        FeedMessage::AddedNode { node: NodeDetails { name: node_name, .. }, ..} if node_name == "Node 2",
+        FeedMessage::AddedChain { name, node_count: 2 } if name == "Initial chain name",
+    );
+
+    // Subscribe a third node. The chain renames, so we're told about the new node but also
+    // about the chain rename.
+    node_tx.send_json_text(node_init_msg(3, "New chain name", "Node 3")).await.unwrap();
+    let feed_messages = feed_rx.recv_feed_messages().await.unwrap();
+    assert_contains_matches!(
+        feed_messages,
+        FeedMessage::AddedNode { node: NodeDetails { name: node_name, .. }, ..} if node_name == "Node 3",
+        FeedMessage::RemovedChain { name } if name == "Initial chain name",
+        FeedMessage::AddedChain { name, node_count: 3 } if name == "New chain name",
+    );
+
+    // Just to be sure, subscribing a fourth node on this chain will still lead to updates
+    // to this feed.
+    node_tx.send_json_text(node_init_msg(4, "New chain name", "Node 4")).await.unwrap();
+    let feed_messages = feed_rx.recv_feed_messages().await.unwrap();
+    assert_contains_matches!(
+        feed_messages,
+        FeedMessage::AddedNode { node: NodeDetails { name: node_name, .. }, ..} if node_name == "Node 4",
+        FeedMessage::AddedChain { name, node_count: 4 } if name == "New chain name",
+    );
+
+}
+
+/// If we add a couple of shards and a node for each, all feeds should be
+/// told about both node chains. If one shard goes away, we should get a
+/// "removed chain" message only for the node connected to that shard.
 #[tokio::test]
 async fn feed_add_and_remove_shard() {
     let mut server = start_server_debug().await;
@@ -130,24 +322,22 @@ async fn feed_add_and_remove_shard() {
 
         // Send a "system connected" message:
         node_tx
-            .send_json_text(json!(
-                {
-                    "id":id,
-                    "ts":"2021-07-12T10:37:47.714666+01:00",
-                    "payload": {
-                        "authority":true,
-                        "chain": format!("Local Testnet {}", id),
-                        "config":"",
-                        "genesis_hash": BlockHash::from_low_u64_ne(id),
-                        "implementation":"Substrate Node",
-                        "msg":"system.connected",
-                        "name":"Alice",
-                        "network_id":"12D3KooWEyoppNCUx8Yx66oV9fJnriXwCcXwDDUA2kj6vnc6iDEp",
-                        "startup_time":"1625565542717",
-                        "version":"2.0.0-07a1af348-aarch64-macos"
-                    },
-                }
-            ))
+            .send_json_text(json!({
+                "id":id,
+                "ts":"2021-07-12T10:37:47.714666+01:00",
+                "payload": {
+                    "authority":true,
+                    "chain": format!("Local Testnet {}", id),
+                    "config":"",
+                    "genesis_hash": BlockHash::from_low_u64_ne(id),
+                    "implementation":"Substrate Node",
+                    "msg":"system.connected",
+                    "name":"Alice",
+                    "network_id":"12D3KooWEyoppNCUx8Yx66oV9fJnriXwCcXwDDUA2kj6vnc6iDEp",
+                    "startup_time":"1625565542717",
+                    "version":"2.0.0-07a1af348-aarch64-macos"
+                },
+            }))
             .await
             .unwrap();
 
@@ -188,6 +378,8 @@ async fn feed_add_and_remove_shard() {
     server.shutdown().await;
 }
 
+/// feeds can subscribe to one chain at a time. They should get the relevant
+/// messages for that chain and no other.
 #[tokio::test]
 async fn feed_can_subscribe_and_unsubscribe_from_chain() {
     use FeedMessage::*;