8

Following on from this question: can-a-temperary-lambda-by-passed-by-reference?

I have a fixed code snippet:

// global variable
std::thread worker_thread;

// Template function
template <typename Functor>
void start_work(const Functor &worker_fn)  // lambda passed by const ref
{
    worker_thread = std::thread([&](){
        worker_fn();
    });
}

This is called like this:

void do_work(int value)
{
    printf("Hello from worker\r\n");
}

int main()
{
    // This lambda is a temporary variable...
    start_work([](int value){ do_work(value) });
}

This seems to work, but I am concerned about passing a temporary lambda into the thread constructor since the thread will run, but the function start_work() will return and the temp-lambda will go out of scope.

However I was looking at the std::thread constructor which is defined:

thread() noexcept; (1) (since C++11)

thread( thread&& other ) noexcept; (2) (since C++11)

template< class Function, class... Args > explicit thread( Function&& f, Args&&... args ); (3) (since C++11)

thread(const thread&) = delete; (4) (since C++11)

So I am assuming that the constructor 3 is called:

template< class Function, class... Args >
explicit thread( Function&& f, Args&&... args );

I struggle to understand what is written here, but it looks like it would try to move the lambda && which I believe is ok for temp variables.

So is what I have done in my code snippet dangerous (i.e. the ref goes out of scope) or correct (i.e. the temp is moved and all is well)? or neither??

An alternative is to just pass my value (make a copy) which is not so bad anyway in this case.

code_fodder
  • 15,263
  • 17
  • 90
  • 167
  • 1
    Upped as promised. A full answer is above my pay grade. – Bathsheba Apr 23 '18 at 14:00
  • 1
    You have a dangling reference. – Passer By Apr 23 '18 at 14:07
  • @FrançoisAndrieux sorry... yes I did mean that, typo, fixed! – code_fodder Apr 23 '18 at 14:07
  • @PasserBy can you expand on that, I thought that the temp would be "moved" by the `std::thread(Function && f ...)` constructor, which I thought was ok? – code_fodder Apr 23 '18 at 14:09
  • 5
    The question is asking if it's ok to pass a `const &` to a temporary to `std::thread`, and it is. But it's not what you are doing. Rather, the problem here is that you are capturing a temporary by reference in a lambda, which isn't the same thing and isn't ok in this context. – François Andrieux Apr 23 '18 at 14:09
  • 2
    Briefly, the lambda may well be moved correctly, but the thing it's capturing will not be. As a charlatan, I think of lambdas as being no more than fancy function objects. – Bathsheba Apr 23 '18 at 14:09
  • Your code does not compile on clang, `error: 'do_work' in capture list does not name a variable`. This capture is not needed. The lambda does not capture anything and I'd say it's just a function pointer whitch explain why there are no dangling pointers. You can effectively write `start_work(do_work)` – Guillaume Gris Apr 23 '18 at 14:11
  • @Bathsheba ah, so I could fix that by using `[=]` or more specifically `[worker_fn](){...}`? – code_fodder Apr 23 '18 at 14:11
  • @GuillaumeGris yes, it is a function pointer - I had to convert from a class function to this example - so I missed that, I'll remove the "do_work" capture, thanks – code_fodder Apr 23 '18 at 14:12
  • @GuillaumeGris also yes, I could write start_work(do_work), but this is just a code snippet, the real function does more, I was just trying to make a minimal example : ) – code_fodder Apr 23 '18 at 14:14
  • This might help you understand the problem: https://stackoverflow.com/questions/7941562/what-is-the-lifetime-of-a-c-lambda-expression – Dan M. Apr 23 '18 at 14:15
  • 1
    @code_fodder Consider making `worker_fn` a forwarding reference (`Functor&& worker_fn`) and initializing your capture with `std::forward`. – François Andrieux Apr 23 '18 at 14:15
  • What I'm trying to say is as long as your lambda does not capture anything, then there are no problems – Guillaume Gris Apr 23 '18 at 14:15
  • 1
    You don't need any of the lambda's in this example. `start_work(do_work);` and `worker_thread = std::thread(worker_fn);` do the same thing (both are missing the `value` parameter for the `do_work` call) – Caleth Apr 23 '18 at 14:20

2 Answers2

8

A temporary is indeed moved but it is the "inner" one, the argument to std::thread.

That temporary holds a reference to the "outer" temporary, the argument to start_work, and whose lifetime ends after start_work has returned.

Thus, your "inner" lambda object holds a reference to an object that may or may not exist during its execution, which is very unsafe.

molbdnilo
  • 64,751
  • 3
  • 43
  • 82
  • ah ok... so in "theory" I could change the inner capture to `[=]` to capture by copy and that should be safe (there will be one copy, but that's better then two)? – code_fodder Apr 23 '18 at 14:20
  • @code_fodder As long as you make sure that the thing you're capturing also hasn't captured any references that can be left dangling, yes. – molbdnilo Apr 23 '18 at 14:55
3

A lambda is an anonymous struct in C++. If we were to translate the snippet to an equivalent one without lambdas, it would become

template <typename Functor>
void start_work(const Functor &worker_fn)
{
    struct lambda {
        const Functor& worker_fn;
        auto operator()() const { worker_fn(); }
    };
    worker_thread = std::thread(lambda{worker_fn});
}

lambda has a non-stack-based const reference as a member, which would dangle as soon as start_work returns, regardless whether the lambda object itself is copied.

Passer By
  • 19,325
  • 6
  • 49
  • 96