Threadsafe network message

Question

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...

Answer 1

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);
        }
 };