UdpSocket::Send(Packet) is now limited to UdpSocket::MaxDatagramSize, so that data is never split into multiple datagrams, which removes a lot of potential major problems

include/SFML/Network/Socket.hpp

@@ -115,19 +115,6 @@ protected :
         Udp  ///< UDP protocol
     };
 
-    ////////////////////////////////////////////////////////////
-    /// \brief Structure holding the data of a pending packet
-    ///
-    ////////////////////////////////////////////////////////////
-    struct PendingPacket
-    {
-        PendingPacket();
-
-        Uint32            Size;         ///< Data of packet size
-        std::size_t       SizeReceived; ///< Number of size bytes received so far
-        std::vector<char> Data;         ///< Data of the packet
-    };
-
     ////////////////////////////////////////////////////////////
     /// \brief Default constructor
     ///
@@ -177,11 +164,6 @@ protected :
     ////////////////////////////////////////////////////////////
     void Close();
 
-    ////////////////////////////////////////////////////////////
-    // Member data
-    ////////////////////////////////////////////////////////////
-    PendingPacket myPendingPacket; ///< Temporary data of the packet currently being received
-
 private :
 
     friend class SocketSelector;

include/SFML/Network/TcpSocket.hpp

@@ -182,7 +182,27 @@ public :
     ////////////////////////////////////////////////////////////
     Status Receive(Packet& packet);
 
+private:
+
     friend class TcpListener;
+
+    ////////////////////////////////////////////////////////////
+    /// \brief Structure holding the data of a pending packet
+    ///
+    ////////////////////////////////////////////////////////////
+    struct PendingPacket
+    {
+        PendingPacket();
+
+        Uint32            Size;         ///< Data of packet size
+        std::size_t       SizeReceived; ///< Number of size bytes received so far
+        std::vector<char> Data;         ///< Data of the packet
+    };
+
+    ////////////////////////////////////////////////////////////
+    // Member data
+    ////////////////////////////////////////////////////////////
+    PendingPacket myPendingPacket; ///< Temporary data of the packet currently being received
 };
 
 } // namespace sf

include/SFML/Network/UdpSocket.hpp

@@ -29,6 +29,7 @@
 // Headers
 ////////////////////////////////////////////////////////////
 #include <SFML/Network/Socket.hpp>
+#include <vector>
 
 
 namespace sf
@@ -146,10 +147,9 @@ public :
     ////////////////////////////////////////////////////////////
     /// \brief Send a formatted packet of data to a remote peer
     ///
-    /// Unlike the other version of Send, this function can accept
+    /// Make sure that the packet size is not greater than
-    /// data sizes greater than UdpSocket::MaxDatagramSize.
+    /// UdpSocket::MaxDatagramSize, otherwise this function will
-    /// If necessary, the data will be split and sent in multiple
+    /// fail and no data will be sent.
-    /// datagrams.
     ///
     /// \param packet        Packet to send
     /// \param remoteAddress Address of the receiver
@@ -180,6 +180,13 @@ public :
     ///
     ////////////////////////////////////////////////////////////
     Status Receive(Packet& packet, IpAddress& remoteAddress, unsigned short& remotePort);
+
+private:
+
+    ////////////////////////////////////////////////////////////
+    // Member data
+    ////////////////////////////////////////////////////////////
+    std::vector<char> myBuffer; ///< Temporary buffer holding the received data in Receive(Packet)
 };
 
 } // namespace sf
@@ -196,23 +203,36 @@ public :
 /// connecting once to a remote host, like TCP sockets,
 /// it can send to and receive from any host at any time.
 ///
+/// It is a datagram protocol: bounded blocks of data (datagrams)
+/// are transfered over the network rather than a continuous
+/// stream of data (TCP). Therefore, one call to Send will always
+/// match one call to Receive (if the datagram is not lost),
+/// with the same data that was sent.
+/// 
 /// The UDP protocol is lightweight but unreliable. Unreliable
-/// means that the data may be corrupted, duplicated, lost or
+/// means that datagrams may be duplicated, be lost or
-/// arrive out of order. UDP is generally used for real-time
+/// arrive reordered. However, if a datagram arrives, its
-/// communication (audio or video streaming, real-time games,
+/// data is guaranteed to be valid.
-/// etc.) where speed is crucial and corrupted data
+///
-/// doesn't matter much.
+/// UDP is generally used for real-time communication
+/// (audio or video streaming, real-time games, etc.) where
+/// speed is crucial and lost data doesn't matter much.
 ///
 /// Sending and receiving data can use either the low-level
 /// or the high-level functions. The low-level functions
-/// process a raw sequence of bytes, and cannot ensure that
+/// process a raw sequence of bytes, whereas the high-level
-/// one call to Send will exactly match one call to Receive
+/// interface uses packets (see sf::Packet), which are easier
-/// at the other end of the socket.
+/// to use and provide more safety regarding the data that is
+/// exchanged. You can look at the sf::Packet class to get
+/// more details about how they work.
 ///
-/// The high-level interface uses packets (see sf::Packet),
+/// It is important to note that UdpSocket is unable to send
-/// which are easier to use and provide more safety regarding
+/// datagrams bigger than MaxDatagramSize. In this case, it
-/// the data that is exchanged. You can look at the sf::Packet
+/// returns an error and doesn't send anything. This applies
-/// class to get more details about how they work.
+/// to both raw data and packets. Indeed, even packets are
+/// unable to split and recompose data, due to the unreliability
+/// of the protocol (dropped, mixed or duplicated datagrams may
+/// lead to a big mess when trying to recompose a packet).
 ///
 /// If the socket is bound to a port, it is automatically
 /// unbound from it when the socket is destroyed. However,

src/SFML/Network/Socket.cpp

@@ -131,19 +131,6 @@ void Socket::Close()
         priv::SocketImpl::Close(mySocket);
         mySocket = priv::SocketImpl::InvalidSocket();
     }
-
-    // Reset the pending packet data
-    myPendingPacket = PendingPacket();
-}
-
-
-////////////////////////////////////////////////////////////
-Socket::PendingPacket::PendingPacket() :
-Size        (0),
-SizeReceived(0),
-Data        ()
-{
-
 }
 
 } // namespace sf

src/SFML/Network/TcpSocket.cpp

@@ -197,8 +197,11 @@ Socket::Status TcpSocket::Connect(const IpAddress& remoteAddress, unsigned short
 ////////////////////////////////////////////////////////////
 void TcpSocket::Disconnect()
 {
-    // Simply close the socket
+    // Close the socket
     Close();
+
+    // Reset the pending packet data
+    myPendingPacket = PendingPacket();
 }
 
 
@@ -354,4 +357,14 @@ Socket::Status TcpSocket::Receive(Packet& packet)
     return Done;
 }
 
+
+////////////////////////////////////////////////////////////
+TcpSocket::PendingPacket::PendingPacket() :
+Size        (0),
+SizeReceived(0),
+Data        ()
+{
+
+}
+
 } // namespace sf

src/SFML/Network/UdpSocket.cpp

@@ -37,7 +37,8 @@ namespace sf
 {
 ////////////////////////////////////////////////////////////
 UdpSocket::UdpSocket() :
-Socket(Udp)
+Socket  (Udp),
+myBuffer(MaxDatagramSize)
 {
 
 }
@@ -154,71 +155,39 @@ 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
+    // UDP is a datagram-oriented protocol (as opposed to TCP which is a stream protocol).
-    // send the packet size first (it would even be a potential source of bug, if
+    // Sending one datagram is almost safe: it may be lost but if it's received, then its data
-    // that size arrives corrupted), but we must split the packet into multiple
+    // is guaranteed to be ok. However, splitting a packet into multiple datagrams would be highly
-    // pieces if data size is greater than the maximum datagram size.
+    // unreliable, since datagrams may be reordered, dropped or mixed between different sources.
+    // That's why SFML imposes a limit on packet size so that they can be sent in a single datagram.
+    // This also removes the overhead associated to packets -- there's no size to send in addition
+    // to the packet's data.
 
     // Get the data to send from the packet
     std::size_t size = 0;
     const char* data = packet.OnSend(size);
 
-    // If size is greater than MaxDatagramSize, the data must be split into multiple datagrams
+    // Send it
-    while (size >= MaxDatagramSize)
+    return Send(data, std::min(size, static_cast<std::size_t>(MaxDatagramSize)), remoteAddress, remotePort);
-    {
-        Status status = Send(data, MaxDatagramSize, remoteAddress, remotePort);
-        if (status != Done)
-            return status;
-
-        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);
 }
 
 
 ////////////////////////////////////////////////////////////
 Socket::Status UdpSocket::Receive(Packet& packet, IpAddress& remoteAddress, unsigned short& remotePort)
 {
-    // First clear the variables to fill
+    // See the detailed comment in Send(Packet) above.
-    packet.Clear();
-    remoteAddress = IpAddress();
-    remotePort    = 0;
-
-    // Receive datagrams
-    std::size_t received = 0;
-    std::size_t size = myPendingPacket.Data.size();
-    do
-    {
-        // Make room in the data buffer for a new datagram
-        myPendingPacket.Data.resize(size + MaxDatagramSize);
-        char* data = &myPendingPacket.Data[0] + size;
 
     // Receive the datagram
-        Status status = Receive(data, MaxDatagramSize, received, remoteAddress, remotePort);
+    std::size_t received = 0;
+    Status status = Receive(&myBuffer[0], myBuffer.size(), received, remoteAddress, remotePort);
+
+    // If we received valid data, we can copy it to the user packet
+    packet.Clear();
+    if ((status == Done) && (received > 0))
+        packet.OnReceive(&myBuffer[0], received);
 
-        // Check for errors
-        if (status != Done)
-        {
-            myPendingPacket.Data.resize(size + received);
     return status;
 }
-    }
-    while (received == MaxDatagramSize);
-
-    // We have received all the packet data: we can copy it to the user packet
-    std::size_t actualSize = size + received;
-    if (actualSize > 0)
-        packet.OnReceive(&myPendingPacket.Data[0], actualSize);
-
-    // Clear the pending packet data
-    myPendingPacket = PendingPacket();
-
-    return Done;
-}
 
 
 } // namespace sf