-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Potential double-ping during discovery #5500
Description
There's still a potential for us to ping the same node twice after the changes in #5483 have been merged - this can happen if we add a node, queue a ping, and then add the node again before the queued ping is actually sent (because we only check sent pings when determining if a node has already been pinged)
One potential way to address this is by adding the ping information to m_sentPings in schedulePing before we actually schedule the ping:
Lines 330 to 338 in b09978b
| void NodeTable::schedulePing(Node const& _node) | |
| { | |
| m_timers.schedule(0, [this, _node](boost::system::error_code const& _ec) { | |
| if (_ec || m_timers.isStopped()) | |
| return; | |
| ping(_node, {}); | |
| }); | |
| } |
A danger with this approach is if there's a delay of the ping not actually executing until after the UDP datagram time to live (1 minute) has passed which means that the ping will already be considered expired when it gets sent out over the wire (and consequently rejected by the receiving party). I don't know much about boost deadline timers but I don't think this can realistically happen, even if there are a lot of boost deadline timers expiring around the same time (since you'd have to execute a lot of handlers to consume 1 minute of wall clock time and I don't think we create that many deadline timers). Additionally, I think that even if this exceptional case happens occasionally, the impact would be the occasional double ping which I think is acceptable.
Another possible way to address this would be to also maintain a list of queued pings, and to check both this list and m_sentPings when determining if a node has already been pinged.