A few more tweaks to TCP failover... seems to be switching back and forth pretty well now!

node/Peer.cpp

@@ -120,25 +120,24 @@ void Peer::receive(
 
 bool Peer::send(const RuntimeEnvironment *_r,const void *data,unsigned int len,uint64_t now)
 {
-	// Note: we'll still use TCP here if that's all we have, but if this
-	// is false we will prefer UDP.
-	bool useTcp = isTcpFailoverTime(_r,now);
-
 	Mutex::Lock _l(_lock);
+	bool useTcp = _isTcpFailoverTime(_r,now);
 
 	std::vector<Path>::iterator p(_paths.begin());
+	if (useTcp) {
+		while ((p->tcp())&&(p != _paths.end()))
+			++p;
+	}
 	if (p == _paths.end())
 		return false;
 
 	uint64_t bestPathLastReceived = p->lastReceived();
 	std::vector<Path>::iterator bestPath = p;
-	bool bestPathIsTcp = p->tcp();
 	while (++p != _paths.end()) {
 		uint64_t lr = p->lastReceived();
-		if ( (lr > bestPathLastReceived) || ((bestPathIsTcp)&&(!useTcp)) ) {
+		if ( (lr > bestPathLastReceived) && ((useTcp)||(!p->tcp())) ) {
 			bestPathLastReceived = lr;
 			bestPath = p;
-			bestPathIsTcp = p->tcp();
 		}
 	}
 
@@ -167,13 +166,11 @@ bool Peer::sendPing(const RuntimeEnvironment *_r,uint64_t now)
 {
 	bool sent = false;
 	SharedPtr<Peer> self(this);
-
+	Mutex::Lock _l(_lock);
-	// In the ping case we will never send TCP unless this returns true.
+	bool useTcp = _isTcpFailoverTime(_r,now);
-	bool useTcp = isTcpFailoverTime(_r,now);
 
 	TRACE("PING %s (useTcp==%d)",_id.address().toString().c_str(),(int)useTcp);
 
-	Mutex::Lock _l(_lock);
 	for(std::vector<Path>::iterator p(_paths.begin());p!=_paths.end();++p) {
 		if ((useTcp)||(!p->tcp())) {
 			p->pinged(now); // we log pings sent even if the send "fails", since what we want to track is when we last tried to ping
@@ -187,32 +184,6 @@ bool Peer::sendPing(const RuntimeEnvironment *_r,uint64_t now)
 	return sent;
 }
 
-bool Peer::isTcpFailoverTime(const RuntimeEnvironment *_r,uint64_t now) const
-	throw()
-{
-	uint64_t lastResync = _r->timeOfLastResynchronize;
-	if ((now - lastResync) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT) {
-		if ((now - _r->timeOfLastPacketReceived) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT)
-			return true;
-
-		uint64_t lastUdpPingSent = 0;
-		uint64_t lastUdpReceive = 0;
-
-		{
-			Mutex::Lock _l(_lock);
-			for(std::vector<Path>::const_iterator p(_paths.begin());p!=_paths.end();++p) {
-				if (p->type() == Path::PATH_TYPE_UDP) {
-					lastUdpPingSent = std::max(lastUdpPingSent,p->lastPing());
-					lastUdpReceive = std::max(lastUdpReceive,p->lastReceived());
-				}
-			}
-		}
-
-		return ( (lastUdpPingSent > lastResync) && (lastUdpPingSent > lastUdpReceive) && ((now - lastUdpPingSent) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT) );
-	}
-	return false;
-}
-
 void Peer::clean(uint64_t now)
 {
 	Mutex::Lock _l(_lock);
@@ -225,4 +196,28 @@ void Peer::clean(uint64_t now)
 	_paths.resize(o);
 }
 
+bool Peer::_isTcpFailoverTime(const RuntimeEnvironment *_r,uint64_t now) const
+	throw()
+{
+	// assumes _lock is locked
+	uint64_t lastResync = _r->timeOfLastResynchronize;
+	if ((now - lastResync) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT) {
+		if ((now - _r->timeOfLastPacketReceived) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT)
+			return true;
+
+		uint64_t lastUdpPingSent = 0;
+		uint64_t lastUdpReceive = 0;
+
+		for(std::vector<Path>::const_iterator p(_paths.begin());p!=_paths.end();++p) {
+			if (p->type() == Path::PATH_TYPE_UDP) {
+				lastUdpPingSent = std::max(lastUdpPingSent,p->lastPing());
+				lastUdpReceive = std::max(lastUdpReceive,p->lastReceived());
+			}
+		}
+
+		return ( (lastUdpPingSent > lastResync) && (lastUdpPingSent > lastUdpReceive) && ((now - lastUdpPingSent) >= ZT_TCP_TUNNEL_FAILOVER_TIMEOUT) );
+	}
+	return false;
+}
+
 } // namespace ZeroTier

node/Peer.hpp

@@ -282,14 +282,6 @@ public:
 		return _lastAnnouncedTo;
 	}
 
-	/**
-	 * @param _r Runtime environment
-	 * @param now Current time
-	 * @return True if it's time to attempt TCP failover (if we have TCP_OUT paths)
-	 */
-	bool isTcpFailoverTime(const RuntimeEnvironment *_r,uint64_t now) const
-		throw();
-
 	/**
 	 * @return Current latency or 0 if unknown (max: 65535)
 	 */
@@ -508,6 +500,9 @@ public:
 	}
 
 private:
+	bool _isTcpFailoverTime(const RuntimeEnvironment *_r,uint64_t now) const
+		throw();
+
 	unsigned char _key[ZT_PEER_SECRET_KEY_LENGTH];
 	Identity _id;
 

node/Topology.hpp

@@ -235,7 +235,7 @@ public:
 			 * than time of last send in order to only count full round trips. */
 			if ( (!_supernodeAddresses.count(p->address())) &&
 			     ((_now - p->lastFrame()) < ZT_PEER_PATH_ACTIVITY_TIMEOUT) &&
-				 ((_now - p->lastDirectReceive()) > ZT_PEER_DIRECT_PING_DELAY) ) {
+			     ((_now - p->lastDirectReceive()) > ZT_PEER_DIRECT_PING_DELAY) ) {
 				p->sendPing(_r,_now);
 			}
 		}
@@ -261,7 +261,7 @@ public:
 			/* For supernodes we always ping even if no frames have been seen, and
 			 * we ping aggressively if pings are unanswered. The limit to this
 			 * frequency is set in the main loop to no more than ZT_STARTUP_AGGRO. */
-			if ( (p->pingUnanswered(_r,_now)) || ((_now - p->lastDirectReceive()) > ZT_PEER_DIRECT_PING_DELAY) )
+			if ( (p->pingUnanswered(_r,_now)) || ((_now - p->lastDirectReceive()) > ZT_PEER_DIRECT_PING_DELAY) || (p->lastDirectReceive() < _r->timeOfLastResynchronize) )
 				p->sendPing(_r,_now);
 		}
 

node/UdpSocket.cpp

@@ -49,6 +49,7 @@
 #endif
 
 // Uncomment to intentionally break UDP in order to test TCP fallback
+// This is here so I can commit it to the repo and drive myself insane.
 //#define ZT_BREAK_UDP
 
 namespace ZeroTier {