Singleton template class to exchange T objects concurrently

Question

Exchanger should provide an exchange method to let two threads at once to exchange their objects. Even tho there's no deadlock some threads got a wrong result. Debugging through I see that one of the two pointers got its value changed with some random one and I'm not able to understand why.

#include <mutex>
#include <condition_variable>
#include <thread>
#include <iostream>
#include <sstream>
#include <vector>
#include <random>
#include <queue>

template <typename T>
class Exchanger {
    inline static Exchanger* instance;
    inline static std::once_flag inited;
    std::mutex mutex;
    std::condition_variable to_exchange;
    T* p1 = nullptr;
    T* p2 = nullptr;
    bool ready = false;

    Exchanger(){};
    ~Exchanger(){};

public:
    static Exchanger* getInstance() {
        std::call_once(inited, []{
            instance = new Exchanger();
        });
        return instance;
    }

    T exchange(T t) {
        std::unique_lock lock(mutex);
        if (!ready) {
            p1 = &t;
            ready = true;
            to_exchange.wait(lock, [this]{return !ready;});
            return *p2;
        } else {
            p2 = &t;
            ready = false;
            to_exchange.notify_one();
            return *p1;
        }

    }

};

int* p = nullptr;

int exchange(int t) {
    p = &t;
}

int main(int argc, char **argv) {

    int x = 10;
    exchange(x);
    std::cout << *p << std::endl;

    std::vector<std::thread> traders;
    for (int i = 0; i < 4; i++) {
        traders.emplace_back([i]{
            std::this_thread::sleep_for(std::chrono::seconds(rand() % 2));
            std::stringstream msg1;
            msg1 << "thread " << std::this_thread::get_id() << " willing to trade " << i << std::endl;
            std::cout << msg1.str();

            std::stringstream msg2;
            msg2 << "thread " << std::this_thread::get_id() << " got " << Exchanger<int>::getInstance()->exchange(i) << std::endl;
            std::cout << msg2.str();
        });
    }

    for (int i = 4; i < 8; i++) {
        traders.emplace_back([i]{
            std::this_thread::sleep_for(std::chrono::seconds(rand() % 2));
            std::stringstream msg1;
            msg1 << "thread " << std::this_thread::get_id() << " willing to trade " << i << std::endl;
            std::cout << msg1.str();

            std::stringstream msg2;
            msg2 << "thread " << std::this_thread::get_id() << " got " << Exchanger<int>::getInstance()->exchange(i) << std::endl;
            std::cout << msg2.str();
        });
    }

    for (auto &t: traders) {
        if (t.joinable()) {
            t.join();
        }
    }

    return 0;
}

Also, shouldn't int exchange destroy the t<int> parameter at the end of the function? Why is the int* p still pointing to the passed value that is in fact a copy?

You're returning a pointer to another thread's expiring local variables. It's a little more exotic than the standard `int i = 3; return &i;` bug, but it works (or, rather, fails to work) in the same way. Consider: where are the swapped values being *stored*? — Sneftel, Jul 19 '19 at 15:26
@Sneftel is the `int exchange` case different from the `thread` one? What's the difference? Isn't `T t` a local variable in fact? When one thread is woke up, is `*p2` pointing to a `null` object? So why is the `int exchange()` case different? — Antonio Santoro, Jul 19 '19 at 15:28
https://stackoverflow.com/questions/25797769/returning-pointer-to-local-variable — Sneftel, Jul 19 '19 at 15:30
You have dangling pointer, as you save address of local variable. — Jarod42, Jul 19 '19 at 15:51

score 0 · Accepted Answer · answered Jul 20 '19 at 00:19

0

Debugging through I see that one of the two pointers got its value changed with some random one and I'm not able to understand why

p1 and p2 are storing an address to a local variable of the function:

    T exchange(T t) {
    std::unique_lock lock(mutex);
    if (!ready) {
        p1 = &t; // <------HERE - STORING ADDRESS TO LOCAL VARIABLE
        ready = true;
        to_exchange.wait(lock, [this]{return !ready;});
        return *p2;
    } else {
        p2 = &t; // <------HERE - STORING ADDRESS TO LOCAL VARIABLE
        ready = false;
        to_exchange.notify_one();
        return *p1;
    }

}

When the function ends, this local variable goes out of scope, causing p1 and p2 to become dangling pointers. accessing dangling pointers is undefined behavior, which also explains the random values you get when debugging. Instead of storing the address, store a copy of the variable.

answered Jul 20 '19 at 00:19

SubMachine

487
3
11

what will happens if T was an UDT without a copy/move-constructor contructor? – Antonio Santoro Jul 20 '19 at 10:54
@AntonioSantoro it depends on what types it contains. As long as it contains only primitives such as double, float etc. a shallow copy that will be performed by the default constructor is enough. In case it contains complex types like pointers or objects with pointers, it must provide an implementation of deep copy constructor as part of the [rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – SubMachine Jul 20 '19 at 13:53
is there a way to not allow exchange for types that do not define a copy-constructor? – Antonio Santoro Jul 20 '19 at 15:55
Not that I know of. It sounds like you should change the design to get what you want – SubMachine Jul 20 '19 at 21:06

Singleton template class to exchange T objects concurrently

1 Answers1