Thread worker simulator

Question

Because I have been writing more multithreaded C++14 code lately, I've developed some tools that help me make sure that my threads are all working as intended. In the real code, it's often difficult to detect race conditions or unintended sequence dependencies among threads, so I have found that temporarily substituting a work simulator (worksim in the code) for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

There are three main pieces in the code below. The first is the Logger class which is more or less a mutex-protected interface to std::cout. It takes a std::stringstream & as input, emits the contents of it to std::cout and the clears the input stringstream.

The second piece is the worksim itself which employs a binomial random number distribution to generate delays that are typically around 25ms, emitting status to the passed Logger object until the stop flag is set. I deliberately do not want to use a conditional variable here because I want to simulate the work that the thread would do.

The third piece is a simple main showing how these are used. It creates 10 threads, waits five seconds and then shuts everything down.

I'm interested in comments on the worksim portion in particular. Specifically,

1. is the intent clear?
2. is the use of a static number generator OK here?
3. is there a better way to handle the logging?

threadplay.cpp

#include <iostream>
#include <atomic>
#include <mutex>
#include <thread>
#include <random>
#include <chrono>
#include <vector>
#include <sstream>

class Logger {
public:
    Logger &operator<<(std::stringstream &ss) {
        std::lock_guard<std::mutex> lock(m);
        std::cout << ss.str();
        ss.str(std::string{});
        return *this;
    }
private:
    std::mutex m;
};

void worksim(Logger &mylog, std::atomic<bool> &stop, int n) {
    using namespace std::chrono_literals;
    static std::mt19937 gen(std::random_device{}());
    std::binomial_distribution<int> dist(50, 0.5);
    std::stringstream ss;
    ss << "Checking " << n << "\n";
    mylog << ss;
    for (auto delay = dist(gen); !stop; delay = dist(gen)) {
        ss << "Thread " << n << " sleeping for " << delay << "ms\n";
        mylog << ss;
        std::this_thread::sleep_for(1ms * delay);
    }
    ss << "Leaving " << n << "\n";
    mylog << ss;
}

int main()
{
    using namespace std::chrono_literals;
    std::atomic<bool> stop{false};
    std::vector<std::thread> th;
    Logger mylog;
    for (int i=0; i<10; ++i) {
        th.push_back(std::thread(worksim, std::ref(mylog), std::ref(stop), i));
    }
    std::this_thread::sleep_for(5s);
    stop = true;
    for (std::thread &thread : th) {
        thread.join();
    }
}

Answer 1

is the intent clear?

I think the intent is obvious.

is the use of a static number generator OK here?

I don't think so. I don't believe that the random number generator is thread safe. So to accesses it from multiple threads (as you are doing) you should lock accesses with a mutex.

is there a better way to handle the logging?

I don't like having to create a std::stringstream seprately and then pass that manually to the logger. I would wrap this up a bit:

class Logger;

class LogStream
{
    Logger&             parent;
    std::stringstream   ss;
    public:
        LogStream(Logger& parent)
            : parent(parent)
        {}
        ~LogStream();
        template<typename T>
        LogStream& operator<<(T const& t) {
            ss << t;
            return *this;
        }
};
class Logger
{
    public:
        Logger(std::ostream& log)
            : log(log)
        {}
        void print(std::stringstream &ss) {
            std::lock_guard<std::mutex> lock(m);
            log << ss.str();
        }
    private:
        std::ostream&   log;
        std::mutex      m;
};
LogStream::~LogStream()
{
    parent.print(ss);
}

Then your usage changes:

std::stringstream ss;
ss << "Checking " << n << "\n";
mylog << ss;

// To 

LogStream(mylog) << "Checking " << n << "\n";

Answer 2

1. Is the intent clear?

Not completely for me. As you're saying

for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

the closest intent I can grasp is, you want to eliminate certain threads by replacing them with worksim threads, to isolate other threads that might be the source of race conditions, dead locks or other multiple threading related problems in the to be tested code.

I'm not a 100% sure if these simplified threads may be really useful for diagnosis of the above mentioned problems. Especially as they won't affect other threads, other than with a random delay and don't touch those synchronization mechanisms left in place. To some degree may be.

2. Is the use of a static number generator OK here?

The instantiation of static local variables will be thread safe. Execution of operations like dist(gen)) will not guarantee thread safety.

The reference documentation about std::binomial_distribution::operator() doesn't tell anything about thread safety, and because gen is a shared resource between threads I would doubt about appropriate results.

3. is there a better way to handle the logging?

The standard I/O output can be fairly slow, and as I mentioned in my comment "The use of a shared logger with the mutex locking may severely influence your threads' runtime behavior".

A generally better approach might be to use the shortest possible lock to pass the log message to the logger (e.g. using a protected queue of std::unique_ptr's), and let the Logger class output these from another background thread (that's what we're using in our production systems).