Hello everybody!
I was writing some code the other day and after reading it again today, I noticed something, that looks like a potential crash to me, but I'm not sure.
So here we go:
Let's say I have a class that runs a thread. In that thread among other thing there is a part that constantly writes stuff to a file. Something like this:
if(m_isLogging)
{
m_mutex.lock();
*m_logFile << string;
m_mutex.unlock();
}
The
m_isLogging variable is an atomic bool, so I can switch the it on and off from a different thread. For that I have a method
startLogging() that looks something like this:
void Worker::startLogging()
{
// Create the file
m_logFile = std::unique_ptr<File>(new File);
// Open the file
if(!m_logFile->open("Log.log"))
{
std::cerr << "Unable to open log!" << std::endl;
return;
}
m_isLogging = true;
}
As far as I see it everything is fine here.
Here comes the stop method:
void Worker::stopLogging()
{
m_mutex.lock();
m_logFile.reset();
m_isLogging = false;
m_mutex.unlock();
}
Now here is my question: As you can see I have the mutex to protect
m_logFile from being deleted while it's still being writen to. But what happens when
stopLogging() (being called from another thread) locks the mutex. Then the thread functions enters the if (because m_isLogging is still true) and put the thread to sleep, because the mutex is locked. Then
stopLogging() finishes deleting the file and releases the mutex. The thread function resumes, because the mutex is unlock and wants to write to an object that isn't there anymore.
Eventhough this might be highly unlikly, because the
reset() and the assignment are really fast operations, this looks like a potetial crash to me. Is that correct?
Now, would it be enough to put a
if(m_logFile != nullptr)
*m_logFile << string;
after the
m_mutex.lock()? Or is there something different/better?
Thanks in advance,
Foaly
