truvorskameikin / udp-discovery-cpp Goto Github PK

When a packet is received, packet_index is checked for increments to see if the userdata needs to be updated:

But here:

packet_index is incremented everytime a packet is sent, even if the userdata didn't change.

Shouldn't it be increased only if the userdata changed?

Now we use uint32_t as the peerId.

Also we use very strange logic to filter and compare discovered peers (see Same method at https://github.com/truvorskameikin/udp-discovery-cpp/blob/master/udp_discovery_peer.hpp#L87) - it uses peer address and maybe peer port. But if we implement sending packets to all the interfaces then this logic will just break showing multiple peers that are just single peer. Let's remove this comparison but rely on comparing peerIds.

This change will require protocol update.

We now use Travis to build the library. Let's switch to actions to build and run the tests.

It is not good to reset packet index while discovery application is running. Packet index reset is sent in single UDP packet that can be lost and other peers will not get it and will keep old and large value of packet index. So the solution is to store packet index in 64 bit integer. Even if the peer will send packet every second it can run without overflow of packet index almost forever.

It safe to name files with the prefix of the library this files correspond to. So renaming of all files is needed. Also rename endpoint to something more suitable.

Has udp-discovery-cpp been tested with Windows 10?

The following software uses udp-discovery-cpp: https://github.com/RetroShare/RetroShare

If I use netstat, I can see that the port is bound properly with the right process but if I check with Wireshark, there's no packet being sent. If I run the linux version then I can see the traffic properly.

Need to add some number of unit tests. Protocol part can be unit-tested, not sure about other parts.

Since we are using UDP broadcast we can have same information about self as for peers.

We can send arbitrary messages over udp to discovered peers. The suggested flow can look like:

• generate random message id
• send message with generated id to peer using udp-discovery-cpp (specify response wait timeout, number of resend attempts)
• wait for response and report either success or fail.

Now the library uses only one thread for both sending and receiving messages. The waiting is done using recvfrom function in the blocking mode (with setting the appropriate timeout). This approach works well but having two threads (one for sending messages and one for receiving) will be much better and we can remove PeerParameters.receive_timeout_ms_ parameter.
The receiving thread can use the default socket timeout.
For doing that we will introduce the ref counter in the env struct. Both threads will increase the counter at start and decrease at exit (also thread will delete env when ref count will reach 0). We assume that threads live longer than udpdiscovery::Peer object and that is why udpdiscovery::Peer object doesn't need to increase ref counter.

Finish writing README file in develop branch. Add:

• "Design" section describing the design of the library.
• "How to use" section describing how to setup and use Endpoint.

Right now if I start two instance of the same application on the same host, it seems that udp-discovery-cpp will see only one of them, at first it seems this happens because because the same socket is used both for listening and sending packets, so the source port is de same as destination port and is the same for all instances, thus as IP:Port is used as identifier for endpoint, seems there is no way to have multiple different endpoints on same host.

This probably can be easily solved using a different socket to send the packets so the source port can be random, and automatically the different endpoints should be recognized.

When I ran ./udp-discovery-example both both test1 and the same with test2 on the same windows computer, they both discovered each other.

It worked also when I ran it with test3 and test4 on another linux computer on the same network.

But the services test1 and test2 didn't discover test3 and test4 and vice versa.

I suspect this issue might be related to having multiple network interfaces on both computers (mostly created by docker). I had similar problem with my simple UDP broadcast implementation using Qt. First two examples below worked only in some cases, but the third one worked well in all my tests. Maybe it could help you to solve the issue also in this project.

This didn't work:

QUdpSocket s;
s.writeDatagram(QByteArray(), QHostAddress::Broadcast, PORT);

This didn't work either:

QUdpSocket s;
QNetworkDatagram datagram(QByteArray(), QHostAddress::Broadcast, PORT);
// Send the packet to all network interfaces
for (int i=0; i<QNetworkInterface::allInterfaces().size(); i++) {
	datagram.setInterfaceIndex(i);
	s.writeDatagram(datagram);
}

This finally worked (got it from here: https://forum.qt.io/post/187963):

// Get network interfaces list
QUdpSocket s;
QList<QNetworkInterface> ifaces = QNetworkInterface::allInterfaces();
for (int i = 0; i < ifaces.size(); i++) {
	auto iface = ifaces[i];
	// Get all IP addresses for the current interface
	QList<QNetworkAddressEntry> addrs = iface.addressEntries();
	// And for any IP address, if it is IPv4 and the interface is active, send the packet
	for (int j = 0; j < addrs.size(); j++) {
		auto addr = addrs[j];
		if ((addr.ip().protocol() == QAbstractSocket::IPv4Protocol) && (addr.broadcast().toString() != "")) {
			s.writeDatagram(QByteArray(), addr.broadcast(), PORT);
		}
	}
}

You can search for documentation of Qt classes here: https://doc.qt.io/qt-5/index.html

Hello,

IsRightTime can set a negative value to to_sleep_until_next_delete_idle, causing the subsequent call to SleepFor to wait for a very long time on windows, and the process getting stuck when calling Stop(true) on the peer.
Indeed, Sleep((DWORD)time_ms) casts the negative value to a very large positive value.

I don't know if IsRightTime is buggy; a possible fix would be replacing "SleepFor(to_sleep_ms);" with "SleepFor(std::max((long)0, to_sleep_ms));", or maybe do the check in the function itself.