0

I have some C++ code that contains roughly this logic:

class wrapper_info {
public:
        bool isConnected();
        void connectedHandler();
        void disconnectedHandler();
protected:
        bool _connected;
}

void wrapper_info::connectedHandler() {
        _connected = true;
}

void wrapper_info::disconnectedHandler() {
        _connected = false;
}

bool wrapper_info::isConnected() {
        return _connected;
}

extern "C"
bool is_connected(void *obj) {
        wrapper_info *wrapper_obj = reinterpret_cast<wrapper_info*>(obj);
        return wrapper_obj->isConnected();
}

For reasons mostly out of my control, different threads (running on different CPU cores) call these functions in the following way.

Thread 1, 2, 3: is_connected(obj)

Thread 2: connectedHandler() when the connection is initiated.

Thread 3 disconnectedHandler() when the connection is broken.

I am thinking there could be issues in the event of repeated calls to connectedHandler() and disconnectedHandler(), issues with the two threads writing to _connected and the writes getting out of order, resulting in the wrong final value. And potentially also issues with polling _connected as well.

My questions are:

  1. What potential issues could actually arise from separate threads polling and modifying the value of _connected?
  2. What options are there to prevent these? Perhaps making _connected a volatile bool might solve issues polling the value. I was also thinking for the issue of threads 2 and 3 modifying its value, perhaps making it an atomic bool and using atomic set operations will be sufficient to prevent issues like out-of-order memory operations. I also know other potential solutions are locks or memory barriers like smb_mb. However, I am not sure what I should use.

Thank you lots.

someone serious
  • 197
  • 1
  • 1
  • 8
  • 3
    *making it an atomic bool and using atomic set operations will be sufficient to prevent issues like out-of-order memory operations* Yes, do that. `volatile` is **not** a thread synchronization technique. – NathanOliver Sep 15 '21 at 18:14
  • 1
    Use std::atomic, or if you need to do more than just set a bool use std::unique_lock together with std::mutex. But you are right you must do something – Pepijn Kramer Sep 15 '21 at 18:15
  • 1
    [Is it ok to read a shared boolean flag without locking it when another thread may set it (at most once)?](https://stackoverflow.com/questions/9200951/is-it-ok-to-read-a-shared-boolean-flag-without-locking-it-when-another-thread-ma) – WhozCraig Sep 15 '21 at 18:18
  • 1
    FYI, there is no such thing as C/C++ code. Your code uses `class` keyword, so it is C++ and not C language. I highly recommend against mixing the two languages, it makes your program more complicated, adds more defects and is harder to maintain. – Thomas Matthews Sep 15 '21 at 19:48

1 Answers1

6

What potential issues could actually arise from separate threads polling and modifying the value of _connected?

It's Undefined Behavior, no matter what.

What options are there to prevent these?

A common solution is to use std::atomic<bool> instead of bool.

There are fancier (and much more complex) ways to ensure synchronization between threads, but std::atomic is an excellent first choice, and not difficult to use correctly.

Perhaps making _connected a volatile bool might solve issues

It won't. volatile does not solve thread synchronization issues.

Drew Dormann
  • 59,987
  • 13
  • 123
  • 180
  • Thank you. I am not synchronizing anything else, just avoiding undefined behavior and making sure there are no bugs, so I suppose with declaring _connected as `std::atomic _connected` it will suffice to use `_connected.store(true/false, std::memory_order_relaxed)` and `_connected.load(std::memory_order_relaxed)`? I figure I don't need other memory orders since I don't care about threads seeing other stuff as having happened before modifying _connected (there is nothing else happening). Is this correct logic? If it helps, this will be run on linux x86_64 – someone serious Sep 16 '21 at 22:14
  • To elaborate in case I am not making sense, my understanding is to use other memory orders such as memory_order_release and memory_order_acquire when in a situation like `int val = 0; _connected.store(true, memory_order_release)` in thread 1 and `_connected.load(memory_order_acquire); int x = val;` in thread 2 (where it is ensured x will be 0). However, I do not have any other dependencies, so I figure I can just use relaxed. – someone serious Sep 16 '21 at 22:19
  • 1
    @someoneserious I recommend the [Ask Question] button if you have a new question. Your question will get much more attention than it will in the comments here. I think it's a good question. – Drew Dormann Sep 17 '21 at 16:12
  • Thank you I will do that – someone serious Sep 17 '21 at 16:40