0

Let's say I start a new thread from a classmethod and pass "this" as a parameter to the lambda of the new thread. If the object is destroyed before the thread uses something from "this", then it's probably undefined behavior. As a simple example:

#include <thread>
#include <iostream>


class Foo
{
public:
    Foo() : m_bar{123} {}

    void test_1()
    {
        std::thread thd = std::thread{[this]()
        {
            std::cout << m_bar << std::endl;
        }};
        thd.detach();
    }

    void test_2()
    {
        test_2(m_bar);
    }
    void test_2(int & bar)
    {
        std::thread thd = std::thread{[this, & bar]()
        {
            std::cout << bar << std::endl;
        }};
        thd.detach();
    }

private:
    int m_bar;
};


int main()
{
    // 1)
    std::thread thd_outer = std::thread{[]()
    {
        Foo foo;
        foo.test_1();
    }};
    thd_outer.detach();

    // 2)
    {
        Foo foo;
        foo.test_1();
    }

    std::cin.get();
}

The outcomes

(For the original project, I have to use VS19, so the exception messages are originally coming from that IDE.)

  1. Starting from thd_outer, test_1 and test_2 are either throwing an exception (Exception thrown: read access violation.) or printing 0 (instead of 123).
  2. Without thd_outer they seem correct.

I've tried the same code with GCC under Linux, and they always print 123.

Which one is the correct behavior? I think it is UB, and in that case all are "correct". If it's not undefined, then why are they different?

I would expect 123 or garbage always because either the object is still valid (123) or was valid but destroyed and a) the memory is not reused yet (123) or reused (garbage). An exception is reasonable but what exactly is throwing it (VS only)?

I've came up with a possible solution to the problem:

class Foo2
{
public:
    Foo2() : m_bar{123} {}
    ~Foo2()
    {
        for (std::thread & thd : threads)
        {
            try
            {
                thd.join();
            }
            catch (const std::system_error & e)
            {
                // handling
            }
        }
    }

    void test_1()
    {
        std::thread thd = std::thread{[this]()
        {
            std::cout << m_bar << std::endl;
        }};
        threads.push_back(std::move(thd));
    }

private:
    int m_bar;
    std::vector<std::thread> threads;
};

Is it a safe solution, without undefined behaviors? Seems like it's working. Is there a better and/or more "standardized" way?

ysylya
  • 3
  • 2
  • 2
    "*If the object is destroyed before the thread uses something from "this", then it's probably undefined behavior*" - not ***probably***, its ***definitely*** Undefined Behavior. And there is no guarantee that an access violation will occur, either. Undefined Behavior is *undefined*, so *anything* can happen. Your "solution" is correct, you have to wait for the thread(s) to finish using `this` before `this` is destroyed. – Remy Lebeau Mar 05 '21 at 18:26
  • Should there be a canonical duplicate for "why is my undefined behaviour not behaving like I expected" questions? – Useless Mar 05 '21 at 18:30
  • @Useless You mean something like [Undefined, unspecified and implementation-defined behavior](https://stackoverflow.com/questions/2397984/undefined-unspecified-and-implementation-defined-behavior)? – JaMiT Mar 05 '21 at 18:36
  • The behaviour of Undefined Behaviour is undefined. – user4581301 Mar 05 '21 at 19:29
  • @JaMiT Something like that. "What is UB" is not identical to "I deliberately invoked UB but am still somehow surprised by the results", but I guess it's close enough. – Useless Mar 07 '21 at 11:42

1 Answers1

0

Forget about member variables or classes. The question then is, how do I make sure that a thread does not use a reference to an object that has been destroyed. Two approaches exist that both effectively ensure that the thread ends before the object is destroyed plus a third one that's more complicated.

  1. Extend the object lifetime to that of the thread. The easiest way is to use dynamic allocation of the object. In addition, to avoid memory leaks, use smart pointers like std::shared_ptr.
  2. Limit the thread runtime to that of the object. Before destroying the object, simply join the thread.
  3. Tell the thread to let go of the object before destroying it. I'll only sketch this, because its the most complicated way, but if you somehow tell the thread that it must not use the object any more, you can then destroy the object without adverse side effects.

That said, one advise: You are sharing an object between (at least) two threads. Accessing it requires synchronization, which is a complex topic in and of itself.

Ulrich Eckhardt
  • 16,572
  • 3
  • 28
  • 55