7

I have a lambda inside a for loop with the loop variable parameter in the lambda. When I run it, I expect the numbers 0-9 to be output. But since it is a lambda, the x doesn't get evaluated immediately.

for (int x = 0; x < n; ++x)
{
    vec.push_back(thread{[&x]() {
        m.lock();
        cout << x << endl;
        m.unlock();
    }});
}

Output:

0
3
3
9
etc.

The solution for other languages would be to create a temporary variable,

for (int x = 0; x < n; ++x)
{
    int tmp = x;
    vec.push_back(thread{[&tmp]() {
        m.lock();
        cout << tmp << endl;
        m.unlock();
}});
}

but that doesn't seem to work.

see Threads receiving wrong parameters

Bonus:

In my search for an answer, I stumbled upon this question Generalizing C++11 Threads class to work with lambda which recommends not using a container that would invalidate the iterators.
Why would that be?

shanfeng
  • 503
  • 2
  • 14
SaulBack
  • 1,406
  • 2
  • 16
  • 31

3 Answers3

14

When you specify the capture, you can choose between capture by value and capture by reference. You have chosen to capture by reference. Capturing by reference means that the variable inside the lambda function is referring to the same object. The implication is that any changes to this variable will be shared and you also need to make sure that the referenced object stays around for the life-time of the lambda function.

You probably meant to capture by values. To do this, you can either replace the capture specification to become [=] or to become [x]. The latter makes sure that only x can be accessed while the former would allow other variables to be accessible.

BTW, I'd recommend not using lock() and unlock() explicitly but rather use one of the lock guards. With this, the body of your loop would look something like this:

vec.push_back(std::thread{[x](){
    std::lock_guard<std::mutex> kerberos(m);
    std::cout << x << "\n";
}});
Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • I feel kind of dumb. I misunderstood what the & was for in lambda expressions. For this oversimplified example, yes a lock_guard would be appropriate. But in the real application, the function does a lot that doesn't need to be locked. In that case a mutex would be appropriate locking and unlocking only around the important part, correct? – SaulBack Sep 17 '12 at 01:23
  • 6
    @SaulBack: No. You should use a `lock_guard` either way. If you only want to lock certain code, use an explicit block with `{}` to enclose the locked code. This is basic RAII coding here. – Nicol Bolas Sep 17 '12 at 01:33
  • 2
    @SaulBack: Especially in complicated code you want to use RAII, e.g., `std::lock_guard`! In this **trivial** example I can see that it isn't needed but the moment it is a bit more complex things get quickly out of hand. If you need to restrict the scope where the lock is held use a scope, i.e., a pair of braces: `{ }`. – Dietmar Kühl Sep 17 '12 at 05:50
6

Capture the parameter by value instead of by reference if you want to make a copy:

vec.push_back(std::thread{[x](){
  m.lock();
  std::cout << x << std::endl;
  m.unlock();
}});

This will copy the value of x at the time the lambda object was created (not at the time the thread was started).

In my search for an answer, I stumbled upon this question Generalizing C++11 Threads class to work with lambda which recommends not using a container that would invalidate the iterators. Why would that be/

Because it's talking about a completely different implementation which directly uses the pthreads library. You're using std::thread, which is designed to work in C++.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • Can you elaborate on why ptheads in a container wouldn't work while a std::thread would? Is it a matter of copy constructors, moving constructors, assignment operators? – SaulBack Sep 17 '12 at 01:26
3

The issue is that you are capturing the x by reference. Thus when x is incremented at the end of each loop, that is reflected in the thread.

You want to capture x by value so that the lambda only uses the value of x when the lambda is created.

for( int x = 0; x < n; ++x)
{               
   vec.push_back(thread{[x](){
      m.lock();
      cout << tmp << endl;
      m.unlock();
   }});
}