Improve protocol-level receiving code (#9617)

src/network/connection.cpp

@@ -1173,7 +1173,9 @@ Connection::Connection(u32 protocol_id, u32 max_packet_size, float timeout,
 	m_bc_peerhandler(peerhandler)
 
 {
-	m_udpSocket.setTimeoutMs(5);
+	/* Amount of time Receive() will wait for data, this is entirely different
+	 * from the connection timeout */
+	m_udpSocket.setTimeoutMs(500);
 
 	m_sendThread->setParent(this);
 	m_receiveThread->setParent(this);

src/network/connectionthreads.cpp

@@ -812,6 +812,14 @@ void *ConnectionReceiveThread::run()
 	ThreadIdentifier);
 	PROFILE(ThreadIdentifier << "ConnectionReceive: [" << m_connection->getDesc() << "]");
 
+	// use IPv6 minimum allowed MTU as receive buffer size as this is
+	// theoretical reliable upper boundary of a udp packet for all IPv6 enabled
+	// infrastructure
+	const unsigned int packet_maxsize = 1500;
+	SharedBuffer<u8> packetdata(packet_maxsize);
+
+	bool packet_queued = true;
+
 #ifdef DEBUG_CONNECTION_KBPS
 	u64 curtime = porting::getTimeMs();
 	u64 lasttime = curtime;
@@ -830,7 +838,7 @@ void *ConnectionReceiveThread::run()
 #endif
 
 		/* receive packets */
-		receive();
+		receive(packetdata, packet_queued);
 
 #ifdef DEBUG_CONNECTION_KBPS
 		debug_print_timer += dtime;
@@ -892,24 +900,11 @@ void *ConnectionReceiveThread::run()
 }
 
 // Receive packets from the network and buffers and create ConnectionEvents
-void ConnectionReceiveThread::receive()
+void ConnectionReceiveThread::receive(SharedBuffer<u8> &packetdata,
+		bool &packet_queued)
 {
-	// use IPv6 minimum allowed MTU as receive buffer size as this is
-	// theoretical reliable upper boundary of a udp packet for all IPv6 enabled
-	// infrastructure
-	unsigned int packet_maxsize = 1500;
-	SharedBuffer<u8> packetdata(packet_maxsize);
-
-	bool packet_queued = true;
-
-	unsigned int loop_count = 0;
-
-	/* first of all read packets from socket */
-	/* check for incoming data available */
-	while ((loop_count < 10) &&
-		(m_connection->m_udpSocket.WaitData(50))) {
-		loop_count++;
 	try {
+		// First, see if there any buffered packets we can process now
 		if (packet_queued) {
 			bool data_left = true;
 			session_t peer_id;
@@ -930,9 +925,12 @@ void ConnectionReceiveThread::receive()
 			packet_queued = false;
 		}
 
+		// Call Receive() to wait for incoming data
 		Address sender;
-			s32 received_size = m_connection->m_udpSocket.Receive(sender, *packetdata,
-				packet_maxsize);
+		s32 received_size = m_connection->m_udpSocket.Receive(sender,
+			*packetdata, packetdata.getSize());
+		if (received_size < 0)
+			return;
 
 		if ((received_size < BASE_HEADER_SIZE) ||
 			(readU32(&packetdata[0]) != m_connection->GetProtocolID())) {
@@ -942,7 +940,7 @@ void ConnectionReceiveThread::receive()
 				<< ", protocol: "
 				<< ((received_size >= 4) ? readU32(&packetdata[0]) : -1)
 				<< std::endl);
-				continue;
+			return;
 		}
 
 		session_t peer_id = readPeerId(*packetdata);
@@ -951,7 +949,7 @@ void ConnectionReceiveThread::receive()
 		if (channelnum > CHANNEL_COUNT - 1) {
 			LOG(derr_con << m_connection->getDesc()
 				<< "Receive(): Invalid channel " << (u32)channelnum << std::endl);
-				throw InvalidIncomingDataException("Channel doesn't exist");
+			return;
 		}
 
 		/* Try to identify peer by sender address (may happen on join) */
@@ -968,49 +966,43 @@ void ConnectionReceiveThread::receive()
 		}
 
 		PeerHelper peer = m_connection->getPeerNoEx(peer_id);
-
 		if (!peer) {
 			LOG(dout_con << m_connection->getDesc()
 				<< " got packet from unknown peer_id: "
 				<< peer_id << " Ignoring." << std::endl);
-				continue;
+			return;
 		}
 
 		// Validate peer address
 
 		Address peer_address;
-
 		if (peer->getAddress(MTP_UDP, peer_address)) {
 			if (peer_address != sender) {
 				LOG(derr_con << m_connection->getDesc()
-						<< m_connection->getDesc()
 					<< " Peer " << peer_id << " sending from different address."
 					" Ignoring." << std::endl);
-					continue;
+				return;
 			}
 		} else {
-
-				bool invalid_address = true;
-				if (invalid_address) {
 			LOG(derr_con << m_connection->getDesc()
-						<< m_connection->getDesc()
-						<< " Peer " << peer_id << " unknown."
+				<< " Peer " << peer_id << " doesn't have an address?!"
 				" Ignoring." << std::endl);
-					continue;
-				}
+			return;
 		}
 
 		peer->ResetTimeout();
 
-			Channel *channel = 0;
-
-			if (dynamic_cast<UDPPeer *>(&peer) != 0) {
-				channel = &(dynamic_cast<UDPPeer *>(&peer)->channels[channelnum]);
+		Channel *channel = nullptr;
+		if (dynamic_cast<UDPPeer *>(&peer)) {
+			channel = &dynamic_cast<UDPPeer *>(&peer)->channels[channelnum];
+		} else {
+			LOG(derr_con << m_connection->getDesc()
+				<< "Receive(): peer_id=" << peer_id << " isn't an UDPPeer?!"
+				" Ignoring." << std::endl);
+			return;
 		}
 
-			if (channel != 0) {
 		channel->UpdateBytesReceived(received_size);
-			}
 
 		// Throw the received packet to channel->processPacket()
 
@@ -1036,14 +1028,15 @@ void ConnectionReceiveThread::receive()
 		catch (ProcessedSilentlyException &e) {
 		}
 		catch (ProcessedQueued &e) {
+			// we set it to true anyway (see below)
+		}
+
+		/* Every time we receive a packet it can happen that a previously
+		 * buffered packet is now ready to process. */
 		packet_queued = true;
 	}
-		}
 	catch (InvalidIncomingDataException &e) {
 	}
-		catch (ProcessedSilentlyException &e) {
-		}
-	}
 }
 
 bool ConnectionReceiveThread::getFromBuffers(session_t &peer_id, SharedBuffer<u8> &dst)
@@ -1189,7 +1182,8 @@ SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Control(Channel *chan
 				m_connection->TriggerSend();
 		} catch (NotFoundException &e) {
 			LOG(derr_con << m_connection->getDesc()
-				<< "WARNING: ACKed packet not in outgoing queue" << std::endl);
+				<< "WARNING: ACKed packet not in outgoing queue"
+				<< " seqnum=" << seqnum << std::endl);
 			channel->UpdatePacketTooLateCounter();
 		}
 

src/network/connectionthreads.h

@@ -101,7 +101,7 @@ public:
 	}
 
 private:
-	void receive();
+	void receive(SharedBuffer<u8> &packetdata, bool &packet_queued);
 
 	// Returns next data from a buffer if possible
 	// If found, returns true; if not, false.