0
\$\begingroup\$

I have started making a very basic networking library in C++. It is built on UDP, with both reliable and unreliable delivery options. So far, I have made the base messaging class, so I will show the header, and the code behind a few functions.

Things to note:

  • While this is thread safe, I'm not actually expecting this to be widely parallelized, hence the atomic variable lock, and not a mutex. There may be contention, but it will be in extremely rare cases.
  • The code you see is not the complete object.

I would primarily like to hear of any logical flaws, or coding errors. The functionality depicted is about all I will need. This library is for my own use, not for public, but I would still like to hear short notes of best practices for a public library. I would not like to hear about packet structure, because that really comes down to what you are using the library for.

//So I can load just the header first, check if message has already been received,
//and allocate appropriate length or discard duplicate messages etc.
struct NetMessageHeader     {
    union
    {
        struct
        {
            bool m_sequenced            : 1;
            bool m_control              : 1;
            bool m_fragment             : 1; //Dubious as to whether I need this
            bool                        : 1; //Unused field
            char m_length_high_nibble   : 4;
            char m_length_low_byte      : 8;
            int m_sequence_number       : 32; //Endianness - caution
        };
        struct
        {
            char m_raw_data[6];
        };
    };
};

class NetMessage
{
private:
    atomic_flag m_locked;

    NetMessageHeader *m_header;
    char *m_message;
    unsigned short m_capacity, m_read_position, m_write_position;
    //write_position points to the next empty element.
    //read_position points to the next element to read.

public:
    static atomic<unsigned int> maximum_message_length;

    NetMessage();
    ~NetMessage();
    unsigned short GetLength();

    bool GetSequence(unsigned int &dest);
    bool _SetSequence(unsigned int value);

    bool IsSequenced();
    bool IsControl();

    bool LoadHeader(char &headerBytes);
    bool LoadData(char &data);  //Loads an array containing all data, including header.

    bool ReserveMemory(unsigned short length);

    bool ReadChar(char &dest);
    bool ReadChars(char &dest, unsigned short length);

    bool WriteChar(char &source);
    bool WriteChars(char &source, unsigned short length);
};

These are the two methods I need to be sure are correct:

bool NetMessage::ReadChars(char &dest, unsigned short length)
{
bool data_read = false;
//Spinlock
while (m_locked.test_and_set(memory_order_seq_cst))
    ;
//Critical section begins
unsigned short remaining_bytes = m_write_position - m_read_position;
if (remaining_bytes >= length)
{
    memcpy((char *)dest, m_message, length);
    m_read_position += length;
    data_read = true;
}
//Critical section ends
m_locked.clear(memory_order_release);
return data_read;
}

bool NetMessage::WriteChars(char &source, unsigned short length)
{
if ((length + HEADER_LENGTH) > maximum_message_length.load(memory_order_acquire))
    return false;
bool data_written = false;
//Spinlock
while (m_locked.test_and_set(memory_order_acquire))
    ;
//Critical section begins
if (m_message == nullptr)  //Empty message, create new one.
{
    m_capacity = NearestPowerOf2(HEADER_LENGTH + length);
    m_message = new char(m_capacity);
    memcpy(m_message + HEADER_LENGTH, (char*) &source, length);
    m_write_position = HEADER_LENGTH + length;
    data_written = true;
}
else if (m_write_position + length < maximum_message_length.load(memory_order_acquire))
{
    int remaining_bytes = m_capacity - m_write_position;
    if (remaining_bytes < length)
    {
        do
        {
            m_capacity *= 2;    //Capacity can exceed maximum message length, write position cannot.
            if (m_capacity & (m_capacity - 1))  //If capacity is not a power of two
                m_capacity = NearestPowerOf2(m_capacity);
        }
        while (remaining_bytes < length);
        char* resized_message = new char(m_capacity);
        memcpy(resized_message, m_message, m_write_position);
        delete[](m_message);
        m_message = resized_message;
        m_header = (NetMessageHeader *)resized_message;
    }
    memcpy(m_message + m_write_position, (char*) &source, 1);
    m_write_position += length;
    data_written = true;
}
//Critical section ends
m_locked.clear(memory_order_release);
}

I am interested in hearing your comments; I've never posted here before, so I don't know (if?) just how bad my coding is...

\$\endgroup\$
2
  • \$\begingroup\$ From the looks of things, I'd give serious consideration to just using the standard RTP packet format, with a few parts (e.g., contributing sources) just zeroed. \$\endgroup\$ Mar 10, 2014 at 23:09
  • \$\begingroup\$ This is mostly an academic exercise for me. I was going to implement this over raw sockets, so I'm not duplicating the length field, but that is just too much hassle. RTP also appears to be a hassle on the Raspberry Pi - I know I didn't say anything about it in the question, but it would be nice to have a networking library I can reuse on my Pi. \$\endgroup\$ Mar 10, 2014 at 23:20

1 Answer 1

3
\$\begingroup\$

Don't use your own spin lock (it is ah huge waste of resources).

while (m_locked.test_and_set(memory_order_seq_cst))
   ;

Use a condition variable.
Also empty blocks are hard to spot and understand if you are going to use them then put a comment that it is deliberately empty.

Never do a manual set and release:

m_locked.test_and_set(memory_order_seq_cst);
// STUFF
m_locked.clear(memory_order_release);

This is not exception safe. Use RAII.

Basically it looks like C code that happens to be wrapped in a class. As it stands not very good as it is easy to use incorrectly. The whole point of C++ is to design the class so it can not be misused (not just write C in a class).

What I would expect as a C++ design.

 // OK. Its unlikely to be this simple but.
 //     Along these lines.
 class Message
 {
     public:
         void sendMessage(int fd)
         {
             write(fd, <Generic Header + Type Info>);
             this->serialize(fd);
         }
         static std::unique_ptr<Message> getMessage(int fd)
         {
             read(fd, <Generic Header + Type Info>);
             auto result Factory::getMessate(<TypeInfo>);
             result->deserialize(fd);
             return result;
         }
     private:
         // Each message type knows how to send itself.
         virtual void serialize(int fd) = 0;

         virtual void deserialize(int fd) = 0;
 };

 class Network
 {
     public:
        void sendMessage(Message const& m)
        {
              // network STUFF
              m.sendMessage(fd);
        }
        std:unique_ptr<Message> getMessage()
        {
              // Network STUFF
              return Message::getMessage(fd);
        }
 };
\$\endgroup\$
3
  • \$\begingroup\$ Oh yeah. Exceptions would totally screw with the critical section. Thanks. Any suggestions in regards to the design? As kolyunya phrased my exact motives so well - "I would like to know community's opinion if I made some design or implementation mistakes, if there could be done some improvements and if the library could be useful in some way to somebody. Any feedback is appreciated." \$\endgroup\$ Mar 11, 2014 at 17:19
  • \$\begingroup\$ Design wise. Your code is like C. You are asking the user to do the work. A better design is to abstract the message. So you have a message object (you don't ask the user to add stuff to the message the message knows what it is). When you send the message to the network it will serialize itself on the send end and deserialize on the receiver end. This should NOT be a multiple step processes. \$\endgroup\$ Mar 11, 2014 at 23:21
  • \$\begingroup\$ To send a message I should not be making multiple calls to the interface in the correct order and checking the status result of these calls. \$\endgroup\$ Mar 11, 2014 at 23:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.