From 2280ff155644823dfbdba9f8dde6ce4d0cb04311 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Mon, 13 Sep 2021 13:31:25 +0100 Subject: [PATCH] Remove node_id_handshake_sockets as unneeded. (#3442) Original commit references a leak, however, none of the clearing operations actually close the socket, they simply decrement the shared_ptr counter. Additionally, removal operations are incorrectly implemented as they delete the shared_ptr in question and every shared_ptr following until the end of the container. --- nano/core_test/node.cpp | 1 - nano/node/transport/tcp.cpp | 35 ----------------------------------- nano/node/transport/tcp.hpp | 5 ----- 3 files changed, 41 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 6d80f6592..0968dc4b8 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -177,7 +177,6 @@ TEST (node, send_single_many_peers) for (auto node : system.nodes) { ASSERT_TRUE (node->stopped); - ASSERT_TRUE (node->network.tcp_channels.node_id_handhake_sockets_empty ()); } } diff --git a/nano/node/transport/tcp.cpp b/nano/node/transport/tcp.cpp index 279180676..a02f8bcbb 100644 --- a/nano/node/transport/tcp.cpp +++ b/nano/node/transport/tcp.cpp @@ -358,7 +358,6 @@ void nano::transport::tcp_channels::stop () } } channels.clear (); - node_id_handshake_sockets.clear (); } bool nano::transport::tcp_channels::max_ip_connections (nano::tcp_endpoint const & endpoint_a) @@ -407,13 +406,11 @@ std::unique_ptr nano::transport::tcp_channels::c nano::lock_guard guard (mutex); channels_count = channels.size (); attemps_count = attempts.size (); - node_id_handshake_sockets_count = node_id_handshake_sockets.size (); } auto composite = std::make_unique (name); composite->add_component (std::make_unique (container_info{ "channels", channels_count, sizeof (decltype (channels)::value_type) })); composite->add_component (std::make_unique (container_info{ "attempts", attemps_count, sizeof (decltype (attempts)::value_type) })); - composite->add_component (std::make_unique (container_info{ "node_id_handshake_sockets", node_id_handshake_sockets_count, sizeof (decltype (node_id_handshake_sockets)::value_type) })); return composite; } @@ -430,12 +427,6 @@ void nano::transport::tcp_channels::purge (std::chrono::steady_clock::time_point // Check if any tcp channels belonging to old protocol versions which may still be alive due to async operations auto lower_bound = channels.get ().lower_bound (node.network_params.network.protocol_version_min); channels.get ().erase (channels.get ().begin (), lower_bound); - - // Cleanup any sockets which may still be existing from failed node id handshakes - node_id_handshake_sockets.erase (std::remove_if (node_id_handshake_sockets.begin (), node_id_handshake_sockets.end (), [this] (auto socket) { - return channels.get ().find (socket->remote_endpoint ()) == channels.get ().end (); - }), - node_id_handshake_sockets.end ()); } void nano::transport::tcp_channels::ongoing_keepalive () @@ -525,28 +516,6 @@ void nano::transport::tcp_channels::update (nano::tcp_endpoint const & endpoint_ } } -bool nano::transport::tcp_channels::node_id_handhake_sockets_empty () const -{ - nano::lock_guard guard (mutex); - return node_id_handshake_sockets.empty (); -} - -void nano::transport::tcp_channels::push_node_id_handshake_socket (std::shared_ptr const & socket_a) -{ - nano::lock_guard guard (mutex); - node_id_handshake_sockets.push_back (socket_a); -} - -void nano::transport::tcp_channels::remove_node_id_handshake_socket (std::shared_ptr const & socket_a) -{ - std::weak_ptr node_w (node.shared ()); - if (auto node_l = node_w.lock ()) - { - nano::lock_guard guard (mutex); - node_id_handshake_sockets.erase (std::remove (node_id_handshake_sockets.begin (), node_id_handshake_sockets.end (), socket_a), node_id_handshake_sockets.end ()); - } -} - void nano::transport::tcp_channels::start_tcp (nano::endpoint const & endpoint_a, std::function const &)> const & callback_a) { if (node.flags.disable_tcp_realtime) @@ -574,7 +543,6 @@ void nano::transport::tcp_channels::start_tcp (nano::endpoint const & endpoint_a channel->set_endpoint (); std::shared_ptr> receive_buffer (std::make_shared> ()); receive_buffer->resize (256); - node_l->network.tcp_channels.push_node_id_handshake_socket (socket); channel->send (message, [node_w, channel, endpoint_a, receive_buffer, callback_a] (boost::system::error_code const & ec, size_t size_a) { if (auto node_l = node_w.lock ()) { @@ -586,7 +554,6 @@ void nano::transport::tcp_channels::start_tcp (nano::endpoint const & endpoint_a { if (auto socket_l = channel->socket.lock ()) { - node_l->network.tcp_channels.remove_node_id_handshake_socket (socket_l); socket_l->close (); } if (node_l->config.logging.network_node_id_handshake_logging ()) @@ -616,7 +583,6 @@ void nano::transport::tcp_channels::start_tcp_receive_node_id (std::shared_ptrnetwork.tcp_channels.remove_node_id_handshake_socket (socket_l); socket_l->close (); } } @@ -688,7 +654,6 @@ void nano::transport::tcp_channels::start_tcp_receive_node_id (std::shared_ptrsocket->type_set (nano::socket::type_t::realtime_response_server); response_server->remote_node_id = channel_a->get_node_id (); response_server->receive (); - node_l->network.tcp_channels.remove_node_id_handshake_socket (socket_l); if (!node_l->flags.disable_initial_telemetry_requests) { diff --git a/nano/node/transport/tcp.hpp b/nano/node/transport/tcp.hpp index e3ed8cb0a..f33bd922a 100644 --- a/nano/node/transport/tcp.hpp +++ b/nano/node/transport/tcp.hpp @@ -105,9 +105,6 @@ namespace transport void start_tcp (nano::endpoint const &, std::function const &)> const & = nullptr); void start_tcp_receive_node_id (std::shared_ptr const &, nano::endpoint const &, std::shared_ptr> const &, std::function const &)> const &); void udp_fallback (nano::endpoint const &, std::function const &)> const &); - void push_node_id_handshake_socket (std::shared_ptr const & socket_a); - void remove_node_id_handshake_socket (std::shared_ptr const & socket_a); - bool node_id_handhake_sockets_empty () const; nano::node & node; private: @@ -228,8 +225,6 @@ namespace transport mi::member>>> attempts; // clang-format on - // This owns the sockets until the node_id_handshake has been completed. Needed to prevent self referencing callbacks, they are periodically removed if any are dangling. - std::vector> node_id_handshake_sockets; std::atomic stopped{ false }; friend class network_peer_max_tcp_attempts_subnetwork_Test;