0

I'm trying to expand with what some people helped me here Call function inside a lambda passed to a thread so my worker class can support a move constructor and a move operator=, but I have the problem that my class is binding this by copy (or reference) to the thread so it can access the class values. Which are several atomic<bool>, a condition_variable and a mutex.

But when I try to move it since the thread is bound to the other condition variable, mutex and atomics, anything I do on it is not working. How can I fix this? Do I need to use a more complex object and move it around instead of a lambda so the thread can have a referenece to it? or is there another alternative. As always help will be really appreciated :).

Here is a snippet (MWE) of the implementation.

class worker {
public:
    template <class Fn, class... Args>
    explicit worker(Fn func, Args... args) {
        t = std::thread(
            [&func, this](Args... cargs) -> void {
                std::unique_lock<std::mutex> lock(mtx);
                while (true) {
                    cond.wait(lock, [&]() -> bool { return ready; });

                    if (terminate)
                        break;

                    func(cargs...);

                    ready = false;
                }
            },
            std::move(args)...);
    }

    worker(worker &&w) : t(std::move(w.t)) { /* here there is trouble  */ }

    worker &operator=(worker &&w) {
        t = std::move(w.t);
        terminate.store(wt.terminate);
        ready.store(wt.ready);
        return *this; 
        /* here too */
    }

    ~worker() {
        terminate = true;
        if (t.joinable()) {
            run_once();
            t.join();
        }
    }

    worker() {}

    void run_once() {
        std::unique_lock<std::mutex> lock(mtx);
        ready = true;
        cond.notify_one();
    }

bool done() { return !ready; }

private:
    std::thread t;
    std::atomic<bool> ready, terminate; // What can I do with all these?
    std::mutex mtx;                     //
    std::condition_variable cond;       //
};

int main() {
    worker t;
    t = worker([]() -> void { cout << "Woof" << endl; });
    t.run_once();
    while(!t.done()) ;
    return 0;
}

Sorry for the big dump of code.

Community
  • 1
  • 1
aram
  • 1,415
  • 13
  • 27
  • Your issue is that you are moving an object that is in use in one thread out from under it in another. Question: why? Create the object dynamically and use smart pointers. – kfsone Jul 27 '16 at 17:48
  • @kfsone Y-yeah that was what I was going with with my second question, wanted to know if there was something more elegant or that I was missing. – aram Jul 27 '16 at 18:08

1 Answers1

2

I would 'fix' it by just saying worker is noncopyable and nonmoveable and leave it to the user of worker to store it as a unique_ptr if they want to move it around. There's nothing wrong with that at all. It's ordinary, actually.

If you absolutely want this class to be movable, you could use the Pimpl design pattern: make a Worker::Impl nested class which worker owns by unique_ptr. The Impl class would be noncopyable and nonmovable and basically be your current worker class. The lambda would have a pointer to the Impl class, not the worker class. The worker class would contain nothing except a unique_ptr to the Impl and functions that forward to the Impl class' functions, and the defaulted copy and move ctors/operators will just work properly for both classes (worker will be copyable but not movable, impl will be noncopyable and nonmovable).

David
  • 27,652
  • 18
  • 89
  • 138
  • Ok, I'll probably do the second one. Thanks. – aram Jul 27 '16 at 18:07
  • @Aram Strongly recommend against it, I only put it there for completeness - to show that it is technically possible. The modern way to 'fix' this problem is to do nothing. Have a noncopyable/nonmovable class and let the user of the class handle it. – David Jul 27 '16 at 19:25
  • what are the cons of that approach if I might ask? – aram Jul 27 '16 at 19:32
  • @Aram 1) If the user doesn't want to move the class they take the hit for the allocation/`unique_ptr` anyway. If you leave it to the user, they only take the hit if they need the hit. 2) It's an ordinary/expected aspect of c++ to potentially need to allocate classes if you want to pass around their ownership, in the case it's nonmoveable. Meaning, it's the c++ equiv of 'pythonic'... C++onic... lol. 3) Complexity. Why make something more complex to solve a problem that's not a problem. – David Jul 27 '16 at 20:06