8
struct Test {
    bool active{true};

    void threadedUpdate() {
        std::this_thread::sleep_for(std::chrono::milliseconds(1));
        if(!active) // crashes here after Test instance is destroyed
            return; 
    }

    Test() { 
        std::thread([this]{ while(true) threadedUpdate(); }).detach();
    }

    ~Test() { 
        // somehow stop the detached thread?
    } 
};

When an instance of Test is initialized, it spawns and detaches an std::thread which runs in background. When the same instance is destroyed, the previously mentioned thread tries to access the active member, which was destroyed along with the instance, causing a crash (and an AddressSanitizer backtrace).

Is there a way to stop the detached thread on ~Test()?

The design is bad. How should a thread running in background until the caller is destroyed be spawned/handled correctly?

user123
  • 8,613
  • 2
  • 25
  • 50
Vittorio Romeo
  • 82,972
  • 25
  • 221
  • 369
  • 3
    As a general rule, you shouldn't detach a thread unless that thread controls the lifespan of any resources it is using. This thread is using `Test` but does not control its lifespan, therefore it should not be detached. – David Schwartz Aug 30 '13 at 19:30

2 Answers2

13

Make the thread a member of the class, and instead of detaching it in the constructor, join it in the destructor. To stop the thread from looping, you can have a boolean inside the class that signals whether the thread should continue running or not (std::atomic<bool> update).

The thread could be executing this: [this] { while (update) threadUpdate(); }.

In the destructor of your class, do update = false, and call thread.join()

user123
  • 8,613
  • 2
  • 25
  • 50
  • @VittorioRomeo - otherwise you'd need a mutex to protect it. According to the standard, non-`atomic` types are never safe to access concurrently without explicit synchronization if at least one of those accesses is a write. – JohannesD Aug 31 '13 at 12:49
  • Another question: would using a member std::future and launching the function with std::async be cleaner instead? I suppose it wouldn't require an explicit join. – Vittorio Romeo Aug 31 '13 at 13:34
  • It could reduce the potential for human error (e.g forgetting to join the thread in the destructor), so it might be worth it. – user123 Aug 31 '13 at 14:34
8

You can't stop detached threads. That's the point of .detach() - you don't have any way to refer to the detached thread anymore, at least as far as the C++ standard specifies. If you want to keep a handle to the thread, store the std::thread and call .join() in the destructor.

  • Is it really necessary to call `.join()` in the destructor if instead of `while(true)` I use `while(active)` and set `active = false;` in the destructor? Shouldn't the thread be stopped then? – Vittorio Romeo Aug 31 '13 at 12:00
  • Another question: would using a member `std::future` and launching the function with `std::async` be cleaner instead? I suppose it wouldn't require an explicit join. – Vittorio Romeo Aug 31 '13 at 13:22
  • Probably it would work, but in general the cleanest solution is to release *all* resources in the destructor, even those that would be released asynchronously. This presumably requires calling join. –  Aug 31 '13 at 21:36