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
You didn't clearly identify the operation that needs to be atomic. From your description of the problem, it's pretty clear that it's not just "write to file", it's the whole "check if logging + write to file" block which must be atomic.
m_mutex.lock();
if(m_isLogging)
*m_logFile << string;
m_mutex.unlock();
Wow thank you very much for those articles! They are very interessting indeed!
I'm not an expert in multithreaded programming, so I'm not sure if I understood everything correctly. I'm having trouble understanding memory-barriers and also adapting the singleton example to my situation.
From reading the second article I understand, that locks aren't nessecarily slow, but from what I understand from the frist article they should be avoided if unnessesary. So my code could be improved to this:
if(m_isLogging)
{
sf::Lock lock(m_mutex);
if(m_isLogging)
{
*m_logFile << string;
}
}
Because now the lock is only created if it's actually needed.
Or am I getting the whole thing wrong? I'm still not 100% sure on all this, but thanks for the help so far :D