Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Deleting variable safely from a different thread  (Read 4779 times)

0 Members and 1 Guest are viewing this topic.

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Deleting variable safely from a different thread
« on: November 13, 2013, 04:40:40 pm »
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

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Deleting variable safely from a different thread
« Reply #1 on: November 13, 2013, 04:50:47 pm »
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();
Laurent Gomila - SFML developer

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Deleting variable safely from a different thread
« Reply #2 on: November 13, 2013, 07:15:23 pm »
Generally, it would be nice if you guaranteed that the file stays open as long as anybody wants to log to it. Depending on the application, it may be problematic if the log output is silently ignored.

This guarantee is only difficult to implement if you use global or static objects. Otherwise, you can ensure that the file closes at the end of main() (at the latest).
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #3 on: November 13, 2013, 10:48:15 pm »
@Laurent: Sorry if I didn't make myself clear. You are right, now that you say it, it seems obvious :D Thanks!

This brings another question to my mind though. Is locking a mutex/creating a Lock (or entering a critical section to be more precise) a lightweight operation? Because if I use the code that you suggested and the thread method locks the mutex every iteration regardless if logging is turned on or off.
Also (just to set things straight in my head) was my assumption that this is a possible crash right?

@Nexus: Like you said it depends on the application. In my application I specifically want to turn logging on and off. But generally speaking you are absolutly right.

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6286
  • Thor Developer
    • View Profile
    • Bromeon
Re: Deleting variable safely from a different thread
« Reply #4 on: November 13, 2013, 11:12:34 pm »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #5 on: November 15, 2013, 11:44:47 am »
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

wintertime

  • Sr. Member
  • ****
  • Posts: 255
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #6 on: November 15, 2013, 03:01:44 pm »
I think it would suffice to declare that m_isLogging as a C++11 atomic and move the mutex locking back inside the "if" to protect the stream.
« Last Edit: November 15, 2013, 03:04:06 pm by wintertime »

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #7 on: November 15, 2013, 04:41:03 pm »
m_isLogging already is a std::atomic<bool>. The possible crash I am trying to prevent here is a when stopLogging() locks the Lock. Then (while the lock is still locked) the thread function reaches the if, enters the block and is put to sleep, because the lock is locked. Once stopLogging() releases the Lock, the thread function resumes and trys to write to the log file and crashes, because the file has been deleted. For a more detailed discribtion see the original post.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: Deleting variable safely from a different thread
« Reply #8 on: November 15, 2013, 05:00:40 pm »
Don't over-engineer it, just put the lock before if(m_isLogging) and come back to this code later if it causes problems (which is very unlikely).
Laurent Gomila - SFML developer

wintertime

  • Sr. Member
  • ****
  • Posts: 255
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #9 on: November 15, 2013, 05:19:46 pm »
Oh I did not see you were deleting the file. :-X
Better just do not delete the file, only flush it and wait with closing till the program ends, because if its used sometimes, someone will want to read it after that sometimes?
Maybe also flush it after writing each line, because loosing the 10 most important lines(the newest) after a crash can be a major time waster if you try to debug the wrong places.

Foaly

  • Sr. Member
  • ****
  • Posts: 453
    • View Profile
Re: Deleting variable safely from a different thread
« Reply #10 on: November 17, 2013, 09:45:28 pm »
Well like I said in my second post this would be right for most application, but in my case I want to be able to switch logging on and off specifically. Don't think about it as error logging. It is rather a class that continously saves data to a file.

I think I am gonna go with what Laurent said and simply leave the lock outside the if. The performance it costs will be unnoticable. Thanks everybody!