3
\$\begingroup\$

I would like to simulate delay over network, for that I have implemented a simple retarder (=introduce random delay while maintaining order of messages) using Boost to handle sending itself. It appears to work as intended. However, since parallel programming is not trivial especially when coupled with networking, I would like to hear feedback more experienced C++ programmer. Mostly, I am concerned about the parallelization(deadlocks, resource leaks) and possible side effects on the communication I might have missed.

class SendRetarder
{
    typedef std::pair<session_ptr, message_t> args_t;//to whom and what
    std::queue<args_t> message_queue;//messages to be sent
    std::mutex m;
    std::condition_variable cv;
    const size_t max_wait = 10000;

    void send_loop()
    {
        while (true)
        {
            boost::system::error_code ec;
            args_t args;
            size_t wait;
            {
                std::unique_lock<std::mutex> lk(m);
                cv.wait(lk, [&] { return !message_queue.empty(); });
                wait = (rand() % max_wait) / message_queue.size();//to prevent unchecked growing
                args = message_queue.front();
                message_queue.pop();
            }
            std::this_thread::sleep_for(std::chrono::milliseconds(wait));
            boost::asio::write(args.first->socket_, boost::asio::buffer(&args.second, sizeof(message_t)), ec);
            if (ec) std::cerr << ec.message().c_str() << std::endl;
        }
    }

public:
    SendRetarder() {
        std::thread(&SendRetarder::send_loop, this).detach();
    }

    void async_send(session_ptr client, message_t& msg)
    {
        {
            std::lock_guard<std::mutex> lk(m);
            message_queue.push(args_t(client, msg));
        }
        cv.notify_one();
    }
};

where message_t is the contents of the message consisting of integer types and session_ptr is a smart pointer wrapper around socket shared with client itself:

struct message_t {
    msg_id_t message_id;
    tag_t message_tag;
    sender_t sender_id;
    union { uint32_t value; bool success; } data;
}

typedef std::shared_ptr<Session> session_ptr;
class Session : public std::enable_shared_from_this<Session> {
    tcp::socket socket_;
    sender_t name_;
    //...
}
\$\endgroup\$
3
\$\begingroup\$

I see a number of things that may help you improve your code.

Consider using existing tools

First, let me note that I've had a need for a similar thing before, but what I eventually used was tc which was tremendously useful for my testing and required essentially no additional code on my part. In my case, although neither of the devices I was testing ran Linux, I put a Linux box between the devices and did all of the data logging from there. Not only did this allow me to arbitrarily delay some number of packets, but also allowed the deliberate injection of bit errors, etc. which very much expanded the possible test repertoire.

Make sure you have all required #includes

The code uses a number of things from both the standard library and from boost but doesn't show the required #includes. Carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation and provide them to users (and reviewers!) of the code.

Lock shared resources before use

The send_loop() routine prints any error message to std::cerr but does so without a lock. This is a problem because std::cerr is a shared resource. If this thread is in the middle of printing a message when another thread writes, you may get interleaved messages. While this is not fatal, it's easily avoided by controlling access to std::cerr via a mutex.

Don't launch a thread without considering object lifetime

The SendRetarder constructor simply launches a detached thread with this line:

std::thread(&SendRetarder::send_loop, this).detach();

However, this has a major problem. What if the object is destroyed after this thread is launched? Everything within the send_loop() routine that relies on object variables is going to be potentially using de-allocated memory and non-existent objects! Prevent this by using shared_from_this and a std::shared_ptr instead of just passing this.

Provide a destructor

What happens to the detached thread if the object is destroyed? Nothing, at the moment, and that's a problem. If the object is destroyed, it seems to me that either there should be a way to signal the detached thread that it's time to quit. One simple way to do that is to close the underlying socket which is one thing that should probably be done in a destructor.

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_int_distribution and friends in the <random> header.

Consider future users

The line that writes the message to the socket looks like this:

boost::asio::write(args.first->socket_,
                   boost::asio::buffer(&args.second, sizeof(message_t)), ec);

That works right now, but what happens if you decide to have a variable size message? For example, consider this:

using message_t = std::string;

Now the sizeof() operator will probably return the size of a pointer rather than the size of the actual object which will lead to maddenly difficult-to-find bugs. I'd recommend adding a size() method to your existing message_t object and then changing the write to look like this:

boost::asio::write(args.first->socket_, 
                   boost::asio::buffer(&args.second, args.second.size())), ec);

Then if the message_t is ever changed to something else, it will be required to implement a size() also. One way to enforce that, if needed, would be to have a message_t base class and have a pure virtual size() but that may be overkill for your purposes.

\$\endgroup\$
  • \$\begingroup\$ Another way to do the size thing is to use a template method whose standard implementation is template<typename T> size_t message_size(const T& t) {return sizeof t; } and which you can specialize for types such as std::string. \$\endgroup\$ – Toby Speight Feb 7 '17 at 17:40
  • \$\begingroup\$ Regarding the size (and future users), I considered using send template for multiple message_t types, this however led to problem I wasnt able to solve - to have one queue for all template instances(message types), and that is the reason I sticked to single non-polymorphic message_t. \$\endgroup\$ – wondra Feb 7 '17 at 18:19
  • \$\begingroup\$ In the end Ifixed resource management problems adding destructor and locking cerr. I also decided not to add mentioned functionality in the end since it suffices for now (and if it wont in future I will try one of more complex tools - the linux box in the middle is such great idea). \$\endgroup\$ – wondra Feb 12 '17 at 18:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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