thread safe random generator?

[bluefrog]

I wrote a random number and string generator, which I intend to use for two purposes. Firstly for generating some test data. The output of the generation is appended to a text file. Secondly I hope to use the thread safe log file writer in the case of exceptions within threads.

My question however is, is the random generator engine (coupled with the uniform distribution object) thread safe ?

This is a (hopefully small enough) snippet of the code:

#include <string>
#include <thread>
#include <random>
#include <algorithm>
#include <iostream>
#include <chrono>
#include <mutex>
#include <fstream>
#include <memory>
#include <cstdint>

class threadsafe_log {
  private:
    std::ofstream  file;
    std::string file_path;
    std::mutex mx;
  public:
    threadsafe_log (const std::string & path) : file_path(path) {  
      file.open(file_path, std::fstream::app); // open file in append mode so nothing is ever overwritten
    }

    ~threadsafe_log() { file.close() ; }

    void write (const std::string & msg) {
      std::lock_guard<std::mutex> lk(mx);
      file << msg;
    }
};

class logger {
  private:
    std::shared_ptr<threadsafe_log> lg;
  public:
    logger (std::shared_ptr<threadsafe_log> logger) : lg(logger) {  }
    void write (const std::string & msg ) { lg->write(msg); }
};

class Random {
  private:
    std::mt19937 eng{std::random_device{}()};
    std::uniform_int_distribution<unsigned int> uniform_dist{0, UINT32_MAX};
  public:
    Random() = default;
    Random(const Random &) = delete;
    Random & operator=(const Random &) = delete;
    Random(std::mt19937::result_type seed) : eng{seed} {}

    unsigned int generate();
    unsigned int generate(unsigned int max);

    std::string gen_random_string (int size);
};

unsigned int Random::generate() {
  return uniform_dist(eng);
}

unsigned int Random::generate(unsigned int max) {
  return uniform_dist(eng, decltype(uniform_dist)::param_type(0, max));
}

std::string Random::gen_random_string (int size ) {

  const std::string VALID_CHARS = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
  std::string str;

  std::generate_n(std::back_inserter(str), size, [&]() {
    return VALID_CHARS[generate(VALID_CHARS.size()-1)];
  });
  return str;
}

void write_to_file_randomly( logger & lg, Random & r, int id, int lines_to_write  ) {
  lines_to_write =  lines_to_write>0 ? lines_to_write : 10; 
  //std::thread::id this_id = std::this_thread::get_id();
  for (int i{0}; i <lines_to_write; i++) {
    lg.write( std::to_string(id) + "," + r.gen_random_string(10) + "\n");
    std::this_thread::sleep_for(std::chrono::milliseconds{r.generate(500)});
  }
}

int main() {
  
  Random r;
  auto file = std::make_shared<threadsafe_log>("test_file.txt");
  logger lg(file);
  
  std::thread t1( ( write_to_file_randomly ), std::ref(lg), std::ref(r), 1, 10);
  std::thread t2( ( write_to_file_randomly ), std::ref(lg), std::ref(r), 2,10);
  
  t1.join();
  t2.join();

  return 0;
}

[gunnerfunner]

Appending a char variable, 'A" for thread t1, B for t2, as an argument to write_to_file_randomly():

void write_to_file_randomly( logger & lg, Random & r, int id, int lines_to_write, const char thread_char  ) {
  lines_to_write =  lines_to_write>0 ? lines_to_write : 10;
  //std::thread::id this_id = std::this_thread::get_id();
  for (int i{0}; i <lines_to_write; i++) {
    lg.write( static_cast<char>(id) + "," + r.gen_random_string(10) + thread_char + "\n");
    std::this_thread::sleep_for(std::chrono::milliseconds{r.generate(500)});
  }
}

A sample run of the output looked like this:

ITv4aK2F78 A
 PtGRWpeJuu B
obk57bOWDu A
vaz04rO5J0 A
Ir3TaRqCun A
 2DS82VuRPk B
EeOVPZ0iHa A
 EugzQIMRUJ B
tW9UK8ylzB A
ZNJwyjXgEj A
 ZTMh0hhVXB B
 me3vvEZ05x B
BtrEmiuCRH A
2CTuv97VLi A
dvFVIFA0Aw A
 Vim7CknzyC B
 bTIKR0DVB9 B
 4b5z2mX733 B
 9wmenYbQCP B
 QYsNH4sYXf B

note the A's and B's at the end of the strings indicating the 2 threads are inter-mingled and the program is not thread-safe.
Also note that some runs of the program print above lines with additional blanks lines in between, not sure where these are coming from -

My guess is that the mutex should be applied directly to the std::ofstream object rather than to the message that the thread_safe log is trying to write. A very simplistic example might be on the following lines:

#include <string>
#include <thread>
#include <iostream>
#include <mutex>
#include <fstream>

std::mutex fileMutex;

void write_to_file(const std::string& fileName, char thread_num)
{
    std::lock_guard<std::mutex> l(fileMutex);
    std::ofstream outFile{fileName, std::fstream::app};
    for (size_t i = 0; i < 10; ++i)
    {
        outFile << i << " " << thread_num << "\n";
    }
}

int main() {

    std::string fileName = std::string {"F:\\test.txt"};

    std::thread t1(write_to_file, fileName, 'A');
    std::thread t2(write_to_file, fileName, 'B');

    t1.join();
    t2.join();
}

As regards thread-safety of the random engine, I found the following 2 links useful:
https://stackoverflow.com/questions/21237905/how-do-i-generate-thread-safe-uniform-random-numbers
https://stackoverflow.com/questions/8813592/c11-thread-safety-of-random-number-generators

[Duthomhas]

No, it is not safe to use a single PRNG across multiple threads without locking as you would any other object for use across multiple threads. That said, though:

  DO NOT USE A SINGLE PRNG ACROSS MULTIPLE THREADS!

Each thread should have its own PRNG. You can easily initialize each one from the OS CSPRNG (/dev/urandom or CryptGenRandom()).

Hope this helps.

[gunnerfunner]

thread_local the PRNG - it'd be slower but thread-safe(r):

#include <string>
#include <thread>
#include <iostream>
#include <mutex>
#include <fstream>
#include <random>
#include <vector>
#include <chrono>
#include <algorithm>

std::mutex fileMutex;

constexpr auto SIZE = 10;
constexpr auto low_bound = 0;
constexpr auto up_bound = 100;


void write_to_file(const std::string& fileName, const char thread_num)
{
    std::lock_guard<std::mutex> l(fileMutex);
    std::ofstream outFile{fileName, std::fstream::app};

    std::this_thread::sleep_for(std::chrono::milliseconds{100});//to move the seed along
    auto seed = std::chrono::system_clock::now().time_since_epoch().count();//seed
    thread_local std::default_random_engine dre(seed);//engine
    std::uniform_int_distribution<int> di(low_bound,up_bound);//distribution
    std::vector<int> data(SIZE);
    std::generate(data.begin(), data.end(), [&di]{ return di(dre);});
    //note: dre is captured by default as it is static (implied by thread-local)
    //https://stackoverflow.com/questions/13827855/capturing-a-static-variable-by-reference-in-a-c11-lambda?rq=1

    for (const auto& elem : data)outFile << elem << " " << thread_num << "\n";
}

int main() {

    std::string fileName = std::string {"F:\\test.txt"};

    std::thread t1(write_to_file, fileName, 'A');
    std::thread t2(write_to_file, fileName, 'B');

    t1.join();
    t2.join();
}

[bluefrog]

Slow reply as I've been away. Thanks for identifying the non thread-safe issues.
So basically 2 things:
1. The PRNG should be a local object
2. The file should be declared each time

I was somewhat confused about thread_local, but then came across the following: https://stackoverflow.com/questions/11983875/what-does-the-thread-local-mean-in-c11

which explains the significance of thread_local.