From 64b710d7cf18d3bacab8c5df71d439bc1a72956a Mon Sep 17 00:00:00 2001 From: dsiganos Date: Tue, 25 May 2021 11:12:20 +0100 Subject: [PATCH] Fix last contacted (#3286) Fix unit test network.last_contacted (#3285) The unit test network.last_contacted has the following problems: * regular keepalive messages sent automatically can interfere with the test * the test should wait for the clock to move at least one tick to eliminate the possibility of the clock not having the time to move forward * the test waits for a the keepalive counter to be incremented and then expects the last_packet_received timestamp to be updated but the timestamp is updated after the counter is incremented, which is a race condition The solution: * send a second keepalive to handle the race condition between setting the timestamp and incrementing the counter * send a third keepalive to handle the case where an automatic keepalive message is already in flight * wait for the clock to move after reading it and before continuing Assumptions: We assume that there will be no more than 1 keelalive in flight. Of course, in theory, there could be multiple keepalives queued up but for the purposes of this test, we assume max 1 keepalive in flight. It is not an unreasonable assumption in this test case. --- nano/core_test/network.cpp | 35 +++++++++++++++++++++++++++++------ nano/node/socket.cpp | 5 +++++ nano/node/socket.hpp | 1 + 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index 304c727e6c..600fdac132 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -133,25 +133,48 @@ TEST (network, send_node_id_handshake_tcp) TEST (network, last_contacted) { nano::system system (1); + auto node0 = system.nodes[0]; ASSERT_EQ (0, node0->network.size ()); + nano::node_config node1_config (nano::get_available_port (), system.logging); node1_config.tcp_incoming_connections_max = 0; // Prevent ephemeral node1->node0 channel repacement with incoming connection auto node1 (std::make_shared (system.io_ctx, nano::unique_path (), node1_config, system.work)); node1->start (); system.nodes.push_back (node1); + auto channel1 = nano::establish_tcp (system, *node1, node0->network.endpoint ()); ASSERT_NE (nullptr, channel1); ASSERT_TIMELY (3s, node0->network.size () == 1); - auto channel2 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); - ASSERT_NE (nullptr, channel2); - // Make sure last_contact gets updated on receiving a non-handshake message - auto timestamp_before_keepalive = channel2->get_last_packet_received (); + + // channel0 is the other side of channel1, same connection different endpoint + auto channel0 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); + ASSERT_NE (nullptr, channel0); + + { + // check that the endpoints are part of the same connection + std::shared_ptr sock0 = channel0->socket.lock (); + std::shared_ptr sock1 = channel1->socket.lock (); + ASSERT_TRUE (sock0->local_endpoint () == sock1->remote_endpoint ()); + ASSERT_TRUE (sock1->local_endpoint () == sock0->remote_endpoint ()); + } + + // capture the state before and ensure the clock ticks at least once + auto timestamp_before_keepalive = channel0->get_last_packet_received (); auto keepalive_count = node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in); + ASSERT_TIMELY (3s, std::chrono::steady_clock::now () > timestamp_before_keepalive); + + // send 3 keepalives + // we need an extra keepalive to handle the race condition between the timestamp set and the counter increment + // and we need one more keepalive to handle the possibility that there is a keepalive already in flight when we start the crucial part of the test + // it is possible that there could be multiple keepalives in flight but we assume here that there will be no more than one in flight for the purposes of this test node1->network.send_keepalive (channel1); - ASSERT_TIMELY (3s, node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) > keepalive_count); + node1->network.send_keepalive (channel1); + node1->network.send_keepalive (channel1); + + ASSERT_TIMELY (3s, node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) >= keepalive_count + 3); ASSERT_EQ (node0->network.size (), 1); - auto timestamp_after_keepalive = channel2->get_last_packet_received (); + auto timestamp_after_keepalive = channel0->get_last_packet_received (); ASSERT_GT (timestamp_after_keepalive, timestamp_before_keepalive); } diff --git a/nano/node/socket.cpp b/nano/node/socket.cpp index fe2b6760ee..ec43f0003a 100644 --- a/nano/node/socket.cpp +++ b/nano/node/socket.cpp @@ -196,6 +196,11 @@ nano::tcp_endpoint nano::socket::remote_endpoint () const return remote; } +nano::tcp_endpoint nano::socket::local_endpoint () const +{ + return tcp_socket.local_endpoint (); +} + nano::server_socket::server_socket (nano::node & node_a, boost::asio::ip::tcp::endpoint local_a, size_t max_connections_a) : socket{ node_a, std::chrono::seconds::max () }, acceptor{ node_a.io_ctx }, diff --git a/nano/node/socket.hpp b/nano/node/socket.hpp index e2002c6790..70c002b179 100644 --- a/nano/node/socket.hpp +++ b/nano/node/socket.hpp @@ -47,6 +47,7 @@ class socket : public std::enable_shared_from_this void close (); boost::asio::ip::tcp::endpoint remote_endpoint () const; + boost::asio::ip::tcp::endpoint local_endpoint () const; /** Returns true if the socket has timed out */ bool has_timed_out () const; /** This can be called to change the maximum idle time, e.g. based on the type of traffic detected. */