Packets handling in UdpSocket is now more robust

git-svn-id: https://sfml.svn.sourceforge.net/svnroot/sfml/branches/sfml2@1490 4e206d99-4929-0410-ac5d-dfc041789085

include/SFML/Network/UdpSocket.hpp

@@ -44,6 +44,14 @@ class SFML_API UdpSocket : public Socket
 {
 public :
 
+    ////////////////////////////////////////////////////////////
+    // Constants
+    ////////////////////////////////////////////////////////////
+    enum
+    {
+        MaxDatagramSize = 65507 ///< The maximum number of bytes that can be sent in a single UDP datagram
+    };
+
     ////////////////////////////////////////////////////////////
     /// \brief Default constructor
     ///
@@ -96,6 +104,10 @@ public :
     ////////////////////////////////////////////////////////////
     /// \brief Send raw data to a remote peer
     ///
+    /// Make sure that \a size is not greater than
+    /// UdpSocket::MaxDatagramSize, otherwise this function will
+    /// fail and no data will be sent.
+    ///
     /// \param data          Pointer to the sequence of bytes to send
     /// \param size          Number of bytes to send
     /// \param remoteAddress Address of the receiver
@@ -113,6 +125,10 @@ public :
     ///
     /// In blocking mode, this function will wait until some
     /// bytes are actually received.
+    /// Be careful to use a buffer which is large enough for
+    /// the data that you intend to receive, if it is too small
+    /// then an error will be returned and *all* the data will
+    /// be lost.
     ///
     /// \param data          Pointer to the array to fill with the received bytes
     /// \param size          Maximum number of bytes that can be received
@@ -130,6 +146,11 @@ public :
     ////////////////////////////////////////////////////////////
     /// \brief Send a formatted packet of data to a remote peer
     ///
+    /// Unlike the other version of Send, this function can accept
+    /// data sizes greater than UdpSocket::MaxDatagramSize.
+    /// If necessary, the data will be split and sent in multiple
+    /// datagrams.
+    ///
     /// \param packet        Packet to send
     /// \param remoteAddress Address of the receiver
     /// \param remotePort    Port of the receiver to send the data to
@@ -146,6 +167,8 @@ public :
     ///
     /// In blocking mode, this function will wait until the whole packet
     /// has been received.
+    /// Warning: this functon doesn't properly handle mixed data
+    /// received from multiple peers.
     ///
     /// \param packet        Packet to fill with the received data
     /// \param remoteAddress Address of the peer that sent the data

src/SFML/Network/TcpSocket.cpp

@@ -208,7 +208,7 @@ Socket::Status TcpSocket::Send(const char* data, std::size_t size)
     // Check the parameters
     if (!data || (size == 0))
     {
-        Err() << "Cannot send data over the network (invalid parameters)" << std::endl;
+        Err() << "Cannot send data over the network (no data to send)" << std::endl;
         return Error;
     }
 
@@ -221,7 +221,7 @@ Socket::Status TcpSocket::Send(const char* data, std::size_t size)
         sent = send(GetHandle(), data + length, sizeToSend - length, 0);
 
         // Check for errors
-        if (sent <= 0)
+        if (sent < 0)
             return priv::SocketImpl::GetErrorStatus();
     }
 
@@ -235,10 +235,10 @@ Socket::Status TcpSocket::Receive(char* data, std::size_t size, std::size_t& rec
     // First clear the variables to fill
     received = 0;
 
-    // Check the parameters
+    // Check the destination buffer
-    if (!data || (size == 0))
+    if (!data)
     {
-        Err() << "Cannot receive data from the network (invalid parameters)" << std::endl;
+        Err() << "Cannot receive data from the network (the destination buffer is invalid)" << std::endl;
         return Error;
     }
 
@@ -265,6 +265,10 @@ Socket::Status TcpSocket::Receive(char* data, std::size_t size, std::size_t& rec
 ////////////////////////////////////////////////////////////
 Socket::Status TcpSocket::Send(Packet& packet)
 {
+    // TCP is a stream protocol, it doesn't preserve messages boundaries.
+    // This means that we have to send the packet size first, so that the
+    // receiver knows the actual end of the packet in the data stream.
+
     // Get the data to send from the packet
     std::size_t size = 0;
     const char* data = packet.OnSend(size);

src/SFML/Network/UdpSocket.cpp

@@ -30,7 +30,7 @@
 #include <SFML/Network/Packet.hpp>
 #include <SFML/Network/SocketImpl.hpp>
 #include <SFML/System/Err.hpp>
-#include <algorithm>
+#include <algorithm>
 #include <string.h>
 
 
@@ -95,28 +95,23 @@ Socket::Status UdpSocket::Send(const char* data, std::size_t size, const IpAddre
     // Create the internal socket if it doesn't exist
     Create();
 
-    // Check the parameters
+    // Make sure that all the data will fit in one datagram
-    if (!data || (size == 0))
+    if (size > MaxDatagramSize)
     {
-        Err() << "Cannot send data over the network (invalid parameters)" << std::endl;
+        Err() << "Cannot send data over the network "
+              << "(the number of bytes to send is greater than sf::UdpSocket::MaxDatagramSize)" << std::endl;
         return Error;
     }
 
     // Build the target address
     sockaddr_in address = priv::SocketImpl::CreateAddress(remoteAddress.ToInteger(), remotePort);
 
-    // Loop until every byte has been sent
+    // Send the data (unlike TCP, all the data is always sent in one call)
-    int sent = 0;
+    int sent = sendto(GetHandle(), data, size, 0, reinterpret_cast<sockaddr*>(&address), sizeof(address));
-    int sizeToSend = static_cast<int>(size);
-    for (int length = 0; length < sizeToSend; length += sent)
-    {
-        // Send a chunk of data
-        sent = sendto(GetHandle(), data + length, sizeToSend - length, 0, reinterpret_cast<sockaddr*>(&address), sizeof(address));
 
-        // Check for errors
+    // Check for errors
-        if (sent <= 0)
+    if (sent < 0)
-            return priv::SocketImpl::GetErrorStatus();
+        return priv::SocketImpl::GetErrorStatus();
-    }
 
     return Done;
 }
@@ -130,10 +125,10 @@ Socket::Status UdpSocket::Receive(char* data, std::size_t size, std::size_t& rec
     remoteAddress = IpAddress();
     remotePort    = 0;
 
-    // Check the parameters
+    // Check the destination buffer
-    if (!data || (size == 0))
+    if (!data)
     {
-        Err() << "Cannot receive data from the network (invalid parameters)" << std::endl;
+        Err() << "Cannot receive data from the network (the destination buffer is invalid)" << std::endl;
         return Error;
     }
 
@@ -145,7 +140,7 @@ Socket::Status UdpSocket::Receive(char* data, std::size_t size, std::size_t& rec
     int sizeReceived = recvfrom(GetHandle(), data, static_cast<int>(size), 0, reinterpret_cast<sockaddr*>(&address), &addressSize);
 
     // Check for errors
-    if (sizeReceived <= 0)
+    if (sizeReceived < 0)
         return priv::SocketImpl::GetErrorStatus();
 
     // Fill the sender informations
@@ -160,27 +155,29 @@ Socket::Status UdpSocket::Receive(char* data, std::size_t size, std::size_t& rec
 ////////////////////////////////////////////////////////////
 Socket::Status UdpSocket::Send(Packet& packet, const IpAddress& remoteAddress, unsigned short remotePort)
 {
+    // As the UDP protocol preserves datagrams boundaries, we don't have to
+    // send the packet size first (it would even be a potential source of bug, if
+    // that size arrives corrupted), but we must split the packet into multiple
+    // pieces if data size is greater than the maximum datagram size.
+
     // Get the data to send from the packet
     std::size_t size = 0;
     const char* data = packet.OnSend(size);
 
-    // First send the packet size
+    // If size is greater than MaxDatagramSize, the data must be split into multiple datagrams
-    Uint32 packetSize = htonl(static_cast<unsigned long>(size));
+    while (size >= MaxDatagramSize)
-    Status status = Send(reinterpret_cast<const char*>(&packetSize), sizeof(packetSize), remoteAddress, remotePort);
-
-    // Make sure that the size was properly sent
-    if (status != Done)
-        return status;
-
-    // Finally send the packet data
-    if (packetSize > 0)
     {
-        return Send(data, size, remoteAddress, remotePort);
+        Status status = Send(data, MaxDatagramSize, remoteAddress, remotePort);
-    }
+        if (status != Done)
-    else
+            return status;
-    {
+
-        return Done;
+        data += MaxDatagramSize;
+        size -= MaxDatagramSize;
     }
+
+    // It is important to send a final datagram with a size < MaxDatagramSize,
+    // even if it is zero, to mark the end of the packet
+    return Send(data, size, remoteAddress, remotePort);
 }
 
 
@@ -192,60 +189,28 @@ Socket::Status UdpSocket::Receive(Packet& packet, IpAddress& remoteAddress, unsi
     remoteAddress = IpAddress();
     remotePort    = 0;
 
-    // We start by getting the size of the incoming packet
+    // Receive datagrams
-    Uint32 packetSize = 0;
     std::size_t received = 0;
-    if (myPendingPacket.SizeReceived < sizeof(myPendingPacket.Size))
+    do
     {
-        // Loop until we've received the entire size of the packet
+        // Make room in the data buffer for a new datagram
-        // (even a 4 bytes variable may be received in more than one call)
+        std::size_t size = myPendingPacket.Data.size();
-        while (myPendingPacket.SizeReceived < sizeof(myPendingPacket.Size))
+        myPendingPacket.Data.resize(size + MaxDatagramSize);
-        {
+        char* data = &myPendingPacket.Data[0] + size;
-            char* data = reinterpret_cast<char*>(&myPendingPacket.Size) + myPendingPacket.SizeReceived;
-            std::size_t size = sizeof(myPendingPacket.Size) - myPendingPacket.SizeReceived;
-            Status status = Receive(data, size, received, remoteAddress, remotePort);
-            myPendingPacket.SizeReceived += received;
 
-            if (status != Done)
+        // Receive the datagram
-                return status;
+        Status status = Receive(data, MaxDatagramSize, received, remoteAddress, remotePort);
-        }
 
-        // The packet size has been fully received
+        // Check for errors
-        packetSize = ntohl(myPendingPacket.Size);
-    }
-    else
-    {
-        // The packet size has already been received in a previous call
-        packetSize = ntohl(myPendingPacket.Size);
-    }
-
-    // Use another address instance for receiving the packet data,
-    // chunks of data coming from a different sender will be discarded (and lost...)
-    IpAddress currentSender;
-    unsigned short currentPort;
-
-    // Loop until we receive all the packet data
-    char buffer[1024];
-    while (myPendingPacket.Data.size() < packetSize)
-    {
-        // Receive a chunk of data
-        std::size_t sizeToGet = std::min(static_cast<std::size_t>(packetSize - myPendingPacket.Data.size()), sizeof(buffer));
-        Status status = Receive(buffer, sizeToGet, received, currentSender, currentPort);
         if (status != Done)
             return status;
-
-        // Append it into the packet
-        if ((currentSender == remoteAddress) && (currentPort == remotePort) && (received > 0))
-        {
-            myPendingPacket.Data.resize(myPendingPacket.Data.size() + received);
-            char* begin = &myPendingPacket.Data[0] + myPendingPacket.Data.size() - received;
-            memcpy(begin, buffer, received);
-        }
     }
+    while (received == MaxDatagramSize);
 
     // We have received all the packet data: we can copy it to the user packet
-    if (!myPendingPacket.Data.empty())
+    std::size_t actualSize = myPendingPacket.Data.size() - MaxDatagramSize + received;
-        packet.OnReceive(&myPendingPacket.Data[0], myPendingPacket.Data.size());
+    if (actualSize > 0)
+        packet.OnReceive(&myPendingPacket.Data[0], actualSize);
 
     // Clear the pending packet data
     myPendingPacket = PendingPacket();