Is this interface thread-safe?

Question

I want to make a control loop interface.

It has should_stop() method to check if loop should break or continue.

It does listen to SIGINT signal(Ctrl+C) and after it gets the signal should_stop() method returns True.

Now it seems like this interface is working.

But I'm not sure this interface is thread-safe.

LoopInterface.h file

#include "signal.h"
#include "pthread.h"

#define LOCK(mutex)     pthread_mutex_lock(&mutex)
#define UNLOCK(mutex)   pthread_mutex_unlock(&mutex)

class LoopInterface {
public:
    LoopInterface(){
        LOCK(_lock_is_signal_registered);
        bool temp = _is_signal_registered;
        UNLOCK(_lock_is_signal_registered);
        if(!temp)
        {
            register_signal();
        }
    }

    bool should_stop()
    {
        LOCK(_lock_should_stop);
        bool temp = _should_stop;
        UNLOCK(_lock_should_stop);
        return _should_stop;
    }

private:

    static void register_signal()
    {
        LOCK(_lock_is_signal_registered);
        _is_signal_registered = true;
        UNLOCK(_lock_is_signal_registered);

        signal(SIGINT, &LoopInterface::signal_handler);
    }

    static void signal_handler(int sig){
        LOCK(_lock_should_stop);
        _should_stop = true;
        UNLOCK(_lock_should_stop);
    }

    static bool _should_stop;
    static bool _is_signal_registered;
    static pthread_mutex_t _lock_should_stop, _lock_is_signal_registered;
};

LoopInterface.cpp file

#include "LoopInterface.h"
bool LoopInterface::_should_stop = false;
bool LoopInterface::_is_signal_registered = false;
pthread_mutex_t LoopInterface::_lock_should_stop;
pthread_mutex_t LoopInterface::_lock_is_signal_registered;

And this is how it is used.

/************Threads*************/
#include "LoopInterface.h"
class A : public LoopInterface{

};

void threadX(){
    A a;
    while(!a.should_stop()){
        //do something...
    }
}

Can you tell me this interface would work thread-safely? or not?

If not, what is the problem?

Additional Problem

There is one more problem on my synchronized code.

Deadlock occurs quite frequently because of calling pthread_mutex_lock in signal_handler while should_stop method is locking the same mutex.

And I found an article stating that thread-related functions should not called in signal handler.

I think I should find another way to synchronize my member variables.

score 1 · Answer 1 · answered Aug 23 '15 at 11:35

1

Clearly, this code is not thread-safe: the variable _should_stop is set by the signal in some uncontrolled thread and read in the LoopInterface running its own thread. There is no synchronization at all. This approach can only be thread-safe if there is exactly one thread.

You can make the code thread-safe (with respect to this specific variable) using

static std::atomic<bool> _should_stop;

Since you are not interested in any other values at this point, you can read the variable with the std::memory_order_relaxed flags.

If the constructor is of LoopInterface is called from multiple threads you also need to synchronize access to _is_signal_registered, of course.

answered Aug 23 '15 at 11:35

Dietmar Kühl

150,225
13
225
380

Thank you. But `atomic` class seems living in c++11 which is not available to me. I will try synchronizing two variables with `mutex`. – JaeJun LEE Aug 23 '15 at 11:48
Dear @Dietmar. I added synchronization into the code. But I have no idea about how to synchronize initializing two mutex objects here. – JaeJun LEE Aug 23 '15 at 12:04
@JeaJunLEE: since you use pthreads you can simply use `pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;` to get the mutex statically initialized. – Dietmar Kühl Aug 23 '15 at 12:08
First, hi @DietmarKühl ))) Second, JaeJunLEE, even if C++ atomics are not available for you, you can still use something else other than mutexes. If you are running your code exclusively on Intel, you can make your variable volatile and enjoy strict memory ordering provided by Intel. I am not advocating for this, just letting you know. Or you can use your OS-provided (or compiler provided) atomic functions. There should be some. – SergeyA Aug 23 '15 at 13:07
Dear @DietmarKühl, I found one problem with synchronizing my member variable. It looks like thread-safe code but it is very likely to make dead-lock problem. Please see my edited question. – JaeJun LEE Aug 23 '15 at 13:20
Rather than calling `LOCK` in the `signal_handler`, creating a thread in `signal_handler` and calling `LOCK` in that thread would solve the problem. Do you think it can be a solution? – JaeJun LEE Aug 23 '15 at 13:35

Is this interface thread-safe?

LoopInterface.h file

LoopInterface.cpp file

Additional Problem

1 Answers1