Split ping() into synchronous ping() and asynchronous schedulePing()

Author: gumb0

libp2p/NodeTable.cpp

@@ -97,7 +97,7 @@ bool NodeTable::addNode(Node const& _node)
         return false;
 
     if (!entry->hasValidEndpointProof())
-        ping(*entry);
+        schedulePing(*entry);
 
     return true;
 }
@@ -118,7 +118,7 @@ bool NodeTable::addKnownNode(
     if (entry->hasValidEndpointProof())
         noteActiveNode(entry->id, entry->endpoint);
     else
-        ping(*entry);
+        schedulePing(*entry);
 
     return true;
 }
@@ -326,32 +326,30 @@ vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeID _target)
 
 void NodeTable::ping(NodeEntry const& _nodeEntry, boost::optional<NodeID> const& _replacementNodeID)
 {
-    m_timers.schedule(0, [this, _nodeEntry, _replacementNodeID](
-                             boost::system::error_code const& _ec) {
+    // don't sent Ping if one is already sent
+    if (contains(m_sentPings, _nodeEntry.id))
+        return;
+
+    NodeIPEndpoint src;
+    src = m_hostNodeEndpoint;
+    PingNode p(src, _nodeEntry.endpoint);
+    p.ts = nextRequestExpirationTime();
+    auto const pingHash = p.sign(m_secret);
+    LOG(m_logger) << p.typeName() << " to " << _nodeEntry.id << "@" << p.destination;
+    m_socket->send(p);
+
+    m_sentPings[_nodeEntry.id] = {chrono::steady_clock::now(), pingHash, _replacementNodeID};
+    if (m_nodeEventHandler && _replacementNodeID)
+        m_nodeEventHandler->appendEvent(_nodeEntry.id, NodeEntryScheduledForEviction);
+}
+
+void NodeTable::schedulePing(NodeEntry const& _nodeEntry)
+{
+    m_timers.schedule(0, [this, _nodeEntry](boost::system::error_code const& _ec) {
         if (_ec || m_timers.isStopped())
             return;
 
-        // don't sent Ping if one is already sent
-        auto sentPing = m_sentPings.find(_nodeEntry.id);
-        if (sentPing != m_sentPings.end())
-        {
-            // we don't need replacement if we're not going to ping
-            if (_replacementNodeID && sentPing->second.replacementNodeID != _replacementNodeID)
-                DEV_GUARDED(x_nodes) { m_allNodes.erase(*_replacementNodeID); }
-            return;
-        }
-
-        NodeIPEndpoint src;
-        src = m_hostNodeEndpoint;
-        PingNode p(src, _nodeEntry.endpoint);
-        p.ts = nextRequestExpirationTime();
-        auto const pingHash = p.sign(m_secret);
-        LOG(m_logger) << p.typeName() << " to " << _nodeEntry.id << "@" << p.destination;
-        m_socket->send(p);
-
-        m_sentPings[_nodeEntry.id] = {chrono::steady_clock::now(), pingHash, _replacementNodeID};
-        if (m_nodeEventHandler && _replacementNodeID)
-            m_nodeEventHandler->appendEvent(_nodeEntry.id, NodeEntryScheduledForEviction);
+        ping(_nodeEntry, {});
     });
 }
 
@@ -360,6 +358,14 @@ void NodeTable::evict(NodeEntry const& _leastSeen, NodeEntry const& _new)
     if (!m_socket->isOpen())
         return;
 
+    // if eviction for _leastSeen already started, just forget about _new
+    auto sentPing = m_sentPings.find(_leastSeen.id);
+    if (sentPing != m_sentPings.end() && sentPing->second.replacementNodeID != _new.id)
+    {
+        DEV_GUARDED(x_nodes) { m_allNodes.erase(_new.id); }
+        return;
+    }
+
     LOG(m_logger) << "Evicting node " << static_cast<Node const&>(_leastSeen);
     ping(_leastSeen, _new.id);
 }
@@ -502,7 +508,10 @@ void NodeTable::onPacketReceived(
                 auto const& optionalReplacementID = sentPing->second.replacementNodeID;
                 if (optionalReplacementID)
                     if (auto replacementNode = nodeEntry(*optionalReplacementID))
+                    {
+                        m_sentPings.erase(replacementNode->id);
                         dropNode(move(replacementNode));
+                    }
 
                 m_sentPings.erase(sentPing);
 

libp2p/NodeTable.h

@@ -218,9 +218,13 @@ class NodeTable : UDPSocketEvents
 
     /// Used to ping a node to initiate the endpoint proof. Used when contacting neighbours if they
     /// don't have a valid endpoint proof (see doDiscover), refreshing buckets and as part of
-    /// eviction process (see evict). Not synchronous - the ping operation is queued via a timer
+    /// eviction process (see evict). Synchronous, has to be called only from the network thread.
     void ping(NodeEntry const& _nodeEntry, boost::optional<NodeID> const& _replacementNodeID = {});
 
+    /// Schedules ping() method to be called from the network thread.
+    /// Not synchronous - the ping operation is queued via a timer.
+    void schedulePing(NodeEntry const& _nodeEntry);
+
     /// Used by asynchronous operations to return NodeEntry which is active and managed by node table.
     std::shared_ptr<NodeEntry> nodeEntry(NodeID _id);