Is my approach to a threadsafe log class awful?

Question

I've been looking around at various approaches to the problem of threadsafe logging, but I haven't seen anything quite like this, so I dunno if it's somehow awful that I haven't noticed due to being a complete newbie to C++, threads and iostreams. It seems to work in the basic tests I've put it through.

Basically I have a Log class (creative, I know...) which has operator<< set up for the standard manipulators, so I can merrily pass in whatever I want.

However, I am aware that something like:

std::cout << "Threads" << " will" << " mess" << " with" << "this." << std::endl;

will potentially get interleaved when multiple threads are writing to cout (or wherever the Log ostream points). So, I have created some manipulators specific to the Log class that let me do this:

Log::log << lock << "Write" << " what" << " I" << " want" << std::endl << unlock;

I just want to know if this is an inherently awful idea, bearing in mind that I am willing to accept that users of the Log class will need to be disciplined with 'lock' and 'unlock'. I considered making 'std::endl' automatically unlock, but that just seems like it would create more headaches... I think undisciplined use should come out in testing anyway, but if anyone can see a way to make such uses cause compile-time errors, that would be nice.

I'd also appreciate any suggestions as to making my code any cleaner.

Here's a cut down version of the class for demonstration purposes; the whole thing has a few more constructors taking stuff like filenames, so not really relevant to the question.

#include <iostream>
#include <thread>
#include <fstream>

class Log{
public:
  //Constructors
  Log(std::ostream & os);
  // Destructor
  ~Log();
  // Input Functions
  Log & operator<<(const std::string & msg);
  Log & operator<<(const int & msg);
  Log & operator<<(std::ostream & (*man)(std::ostream &)); // Handles manipulators like endl.
  Log & operator<<(std::ios_base & (*man)(std::ios_base &)); // Handles manipulators like hex.
  Log & operator<<(Log & (*man)(Log &)); // Handles custom Log manipulators like lock and unlock.
  friend Log & lock(Log & log); // Locks the Log for threadsafe output.
  friend Log & unlock(Log & log); // Unlocks the Log once threadsafe output is complete.
private:
  std::fstream logFile;
  std::ostream & logStream;
  std::mutex guard;
};

// Log class manipulators.
Log & lock(Log & log); // Locks the Log for threadsafe output.
Log & unlock(Log & log); // Unlocks the Log once threadsafe output is complete.

void threadUnsafeTask(int * input, Log * log);
void threadSafeTask(int * input, Log * log);

int main(){
  int one(1), two(2);
  Log log(std::cout);
  std::thread first(threadUnsafeTask, &one, &log);
  std::thread second(threadUnsafeTask, &two, &log);
  first.join();
  second.join();
  std::thread third(threadSafeTask, &one, &log);
  std::thread fourth(threadSafeTask, &two, &log);
  third.join();
  fourth.join();
  return 0;
}

void threadUnsafeTask(int * input, Log * log){
  *log << "Executing" << " thread '" << *input << "', " << "expecting " << "interruptions " << "frequently." << std::endl;
}

void threadSafeTask(int * input, Log * log){
  *log << lock << "Executing" << " thread '" << *input << "', " << "not expecting " << "interruptions." << std::endl << unlock;
}

// Constructors (Most left out as irrelevant)
Log::Log(std::ostream & os): logFile(), logStream(logFile), guard(){
  logStream.rdbuf(os.rdbuf());
}

// Destructor
Log::~Log(){
  logFile.close();
}

// Output Operators
Log & Log::operator<<(const std::string & msg){
  logStream << msg;
  return *this;
}

Log & Log::operator<<(const int & msg){
  logStream << msg;
  return *this;
}

Log & Log::operator<<(std::ostream & (*man)(std::ostream &)){
  logStream << man;
  return *this;
}

Log & Log::operator<<(std::ios_base & (*man)(std::ios_base &)){
  logStream << man;
  return *this;
}

Log & Log::operator<<(Log & (*man)(Log &)){
  man(*this);
  return *this;
}

// Manipulator functions.
Log & lock(Log & log){
  log.guard.lock();
  return log;
}

Log & unlock(Log & log){
  log.guard.unlock();
  return log;
}

It works for me on Ubuntu 12.04 g++, compiled with:

g++ LogThreadTest.cpp -o log -std=c++0x -lpthread

The bits relevant to making custom manipulators was shamelessly cribbed from here but don't blame them for my incompetent copypasta.

Answer 1

It's a bad idea. Imagine this:

void foo()
{
    throw std::exception();
}

log << lock << "Write" << foo() << " I" << " want" << std::endl << unlock;
                          ^
                          exception!

This leaves your Log locked. Which is bad as other threads might be waiting to lock. This also happens everytime you simply forget to do the unlock. You should use RAII here:

// just providing a scope
{
    std::lock_guard<Log> lock(log);
    log << "Write" << foo() << " I" << " want" << std::endl;
}

You'd need to adjust your lock and unlock methods to have the signatures void lock() and void unlock() and make them member functions of the class Log.

---

On the other hand, that's rather bulky. Note that in C++11, using std::cout is thread-safe. So you can easily do

std::stringstream stream;
stream << "Write" << foo() << " I" << " want" << std::endl;
std::cout << stream.str();

which is completely free of additional locks.