Do not directly stop nodes in tests

nano/core_test/network.cpp

@@ -569,7 +569,7 @@ TEST (network, ipv6_bind_send_ipv4)
 TEST (network, endpoint_bad_fd)
 {
 	nano::test::system system (1);
-	system.nodes[0]->stop ();
+	system.stop_node (*system.nodes[0]);
 	auto endpoint (system.nodes[0]->network.endpoint ());
 	ASSERT_TRUE (endpoint.address ().is_loopback ());
 	// The endpoint is invalidated asynchronously

nano/core_test/node.cpp

@@ -40,8 +40,6 @@ TEST (node, stop)
 {
 	nano::test::system system (1);
 	ASSERT_NE (system.nodes[0]->wallets.items.end (), system.nodes[0]->wallets.items.begin ());
-	system.nodes[0]->stop ();
-	system.io_ctx->run ();
 	ASSERT_TRUE (true);
 }
 
@@ -77,9 +75,10 @@ TEST (node, block_store_path_failure)
 	auto path (nano::unique_path ());
 	nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits<unsigned>::max () };
 	auto node (std::make_shared<nano::node> (io_ctx, system.get_available_port (), path, pool));
+	system.register_node (node);
 	ASSERT_TRUE (node->wallets.items.empty ());
-	node->stop ();
 }
+
 #if defined(__clang__) && defined(__linux__) && CI
 // Disable test due to instability with clang and actions
 TEST (node_DeathTest, DISABLED_readonly_block_store_not_exist)
@@ -102,16 +101,13 @@ TEST (node_DeathTest, readonly_block_store_not_exist)
 TEST (node, password_fanout)
 {
 	nano::test::system system;
-	auto io_ctx = std::make_shared<boost::asio::io_context> ();
-	auto path (nano::unique_path ());
 	nano::node_config config;
 	config.peering_port = system.get_available_port ();
 	nano::work_pool pool{ nano::dev::network_params.network, std::numeric_limits<unsigned>::max () };
 	config.password_fanout = 10;
-	nano::node node (io_ctx, path, config, pool);
+	auto & node = *system.add_node (config);
 	auto wallet (node.wallets.create (100));
 	ASSERT_EQ (10, wallet->store.password.values.size ());
-	node.stop ();
 }
 
 TEST (node, balance)
@@ -284,8 +280,6 @@ TEST (node, auto_bootstrap)
 	ASSERT_TIMELY_EQ (5s, node1->ledger.cache.block_count, 3);
 	// Confirmation for all blocks
 	ASSERT_TIMELY_EQ (5s, node1->ledger.cache.cemented_count, 3);
-
-	node1->stop ();
 }
 
 TEST (node, auto_bootstrap_reverse)
@@ -329,8 +323,6 @@ TEST (node, auto_bootstrap_age)
 	ASSERT_TIMELY (10s, node0->stats.count (nano::stat::type::bootstrap, nano::stat::detail::initiate_legacy_age, nano::stat::dir::out) >= 3);
 	// More attempts with frontiers age
 	ASSERT_GE (node0->stats.count (nano::stat::type::bootstrap, nano::stat::detail::initiate_legacy_age, nano::stat::dir::out), node0->stats.count (nano::stat::type::bootstrap, nano::stat::detail::initiate, nano::stat::dir::out));
-
-	node1->stop ();
 }
 
 TEST (node, merge_peers)
@@ -2843,7 +2835,7 @@ TEST (node, peers)
 	ASSERT_TRUE (store.peer.exists (store.tx_begin_read (), endpoint_key));
 
 	// Stop the peer node and check that it is removed from the store
-	node1->stop ();
+	system.stop_node (*node1);
 
 	// TODO: In `tcp_channels::store_all` we skip store operation when there are no peers present,
 	// so the best we can do here is check if network is empty
@@ -2873,7 +2865,7 @@ TEST (node, peer_cache_restart)
 		auto list (node2->network.list (2));
 		ASSERT_EQ (node1->network.endpoint (), list[0]->get_endpoint ());
 		ASSERT_EQ (1, node2->network.size ());
-		node2->stop ();
+		system.stop_node (*node2);
 	}
 	// Restart node
 	{
@@ -2896,7 +2888,7 @@ TEST (node, peer_cache_restart)
 		auto list (node3->network.list (2));
 		ASSERT_EQ (node1->network.endpoint (), list[0]->get_endpoint ());
 		ASSERT_EQ (1, node3->network.size ());
-		node3->stop ();
+		system.stop_node (*node3);
 	}
 }
 

nano/core_test/rep_crawler.cpp

@@ -220,7 +220,7 @@ TEST (rep_crawler, rep_remove)
 	ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 2);
 
 	// When Rep2 is stopped, it should not be found as principal representative anymore
-	node_rep2->stop ();
+	system.stop_node (*node_rep2);
 	ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1);
 
 	// Now only genesisRep should be found:
@@ -239,7 +239,7 @@ TEST (rep_crawler, rep_connection_close)
 	// Add working representative (node 2)
 	system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv);
 	ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 1);
-	node2.stop ();
+	system.stop_node (node2);
 	// Remove representative with closed channel
 	ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 0);
 }

nano/core_test/socket.cpp

@@ -103,8 +103,6 @@ TEST (socket, max_connections)
 	ASSERT_TIMELY_EQ (5s, get_tcp_accept_successes (), 5);
 	ASSERT_TIMELY_EQ (5s, connection_attempts, 8); // connections initiated by the client
 	ASSERT_TIMELY_EQ (5s, server_sockets.size (), 5); // connections accepted by the server
-
-	node->stop ();
 }
 
 TEST (socket, max_connections_per_ip)
@@ -162,8 +160,6 @@ TEST (socket, max_connections_per_ip)
 	ASSERT_TIMELY_EQ (5s, get_tcp_accept_successes (), max_ip_connections);
 	ASSERT_TIMELY_EQ (5s, get_tcp_max_per_ip (), 1);
 	ASSERT_TIMELY_EQ (5s, connection_attempts, max_ip_connections + 1);
-
-	node->stop ();
 }
 
 TEST (socket, limited_subnet_address)
@@ -283,8 +279,6 @@ TEST (socket, max_connections_per_subnetwork)
 	ASSERT_TIMELY_EQ (5s, get_tcp_accept_successes (), max_subnetwork_connections);
 	ASSERT_TIMELY_EQ (5s, get_tcp_max_per_subnetwork (), 1);
 	ASSERT_TIMELY_EQ (5s, connection_attempts, max_subnetwork_connections + 1);
-
-	node->stop ();
 }
 
 TEST (socket, disabled_max_peers_per_ip)
@@ -344,8 +338,6 @@ TEST (socket, disabled_max_peers_per_ip)
 	ASSERT_TIMELY_EQ (5s, get_tcp_accept_successes (), max_ip_connections + 1);
 	ASSERT_TIMELY_EQ (5s, get_tcp_max_per_ip (), 0);
 	ASSERT_TIMELY_EQ (5s, connection_attempts, max_ip_connections + 1);
-
-	node->stop ();
 }
 
 TEST (socket, disconnection_of_silent_connections)
@@ -399,8 +391,6 @@ TEST (socket, disconnection_of_silent_connections)
 	ASSERT_EQ (0, get_tcp_io_timeout_drops ());
 	// Asserts the silent checker worked.
 	ASSERT_EQ (1, get_tcp_silent_connection_drops ());
-
-	node->stop ();
 }
 
 TEST (socket, drop_policy)

nano/core_test/telemetry.cpp

@@ -334,16 +334,14 @@ TEST (telemetry, disconnected)
 	nano::node_flags node_flags;
 	auto node_client = system.add_node (node_flags);
 	auto node_server = system.add_node (node_flags);
-
 	nano::test::wait_peer_connections (system);
-
 	auto channel = node_client->network.find_node_id (node_server->get_node_id ());
 	ASSERT_NE (nullptr, channel);
 
 	// Ensure telemetry is available before disconnecting
 	ASSERT_TIMELY (5s, node_client->telemetry.get_telemetry (channel->get_endpoint ()));
 
-	node_server->stop ();
+	system.stop_node (*node_server);
 	ASSERT_TRUE (channel);
 
 	// Ensure telemetry from disconnected peer is removed

nano/test_common/system.cpp

@@ -211,6 +211,24 @@ std::shared_ptr<nano::node> nano::test::system::make_disconnected_node (std::opt
 	return node;
 }
 
+void nano::test::system::register_node (std::shared_ptr<nano::node> const & node)
+{
+	debug_assert (std::find (nodes.begin (), nodes.end (), node) == nodes.end ());
+	nodes.push_back (node);
+}
+
+void nano::test::system::stop_node (nano::node & node)
+{
+	auto stopped = std::async (std::launch::async, [&node] () {
+		node.stop ();
+	});
+	auto ec = poll_until_true (5s, [&] () {
+		auto status = stopped.wait_for (0s);
+		return status == std::future_status::ready;
+	});
+	debug_assert (!ec);
+}
+
 void nano::test::system::ledger_initialization_set (std::vector<nano::keypair> const & reps, nano::amount const & reserve)
 {
 	nano::block_hash previous = nano::dev::genesis->hash ();

nano/test_common/system.hpp

@@ -25,6 +25,8 @@ namespace test
 		system (uint16_t, nano::transport::transport_type = nano::transport::transport_type::tcp, nano::node_flags = nano::node_flags ());
 		~system ();
 
+		void stop ();
+
 		void ledger_initialization_set (std::vector<nano::keypair> const & reps, nano::amount const & reserve = 0);
 		void generate_activity (nano::node &, std::vector<nano::account> &);
 		void generate_mass_activity (uint32_t, nano::node &);
@@ -50,7 +52,6 @@ namespace test
 		std::error_code poll (std::chrono::nanoseconds const & sleep_time = std::chrono::milliseconds (50));
 		std::error_code poll_until_true (std::chrono::nanoseconds deadline, std::function<bool ()>);
 		void delay_ms (std::chrono::milliseconds const & delay);
-		void stop ();
 		void deadline_set (std::chrono::duration<double, std::nano> const & delta);
 		/*
 		 * Convenience function to get a reference to a node at given index. Does bound checking.
@@ -61,6 +62,8 @@ namespace test
 
 		// Make an independent node that uses system resources but is not part of the system node list and does not automatically connect to other nodes
 		std::shared_ptr<nano::node> make_disconnected_node (std::optional<nano::node_config> opt_node_config = std::nullopt, nano::node_flags = nano::node_flags ());
+		void register_node (std::shared_ptr<nano::node> const &);
+		void stop_node (nano::node &);
 
 		/*
 		 * Returns default config for node running in test environment