Why is this piece of C++ code not synchronized

Question

I am learning to write multithreading applications. So share I run into trouble anytime I want my threads to access even the simples shared resources, despite using mutex.

For example, consider this code:

using namespace std;
mutex mu;
std::vector<string> ob;

void addSomeAValues(){
    mu.lock();    
    for(int a=0; a<10; a++){
        ob.push_back("A" + std::to_string(a));
        usleep(300);    
    }
    mu.unlock();
}

void addSomeBValues(){    
    mu.lock();    
    for(int b=0; b<10; b++){            
        ob.push_back("B" + std::to_string(b));        
        usleep(300);    
    }   
    mu.unlock();
}

int main() {    
    std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now();        
    thread t0(addSomeAValues);        
    thread t1(addSomeBValues);    
    std::chrono::steady_clock::time_point end = std::chrono::steady_clock::now();

    t0.join();
    t1.join();

    //Display the results
    cout << "Code Run Complete; results: \n";    
    for(auto k : ob){
        cout << k <<endl;
    }
    //Code running complete, report the time it took
    typedef std::chrono::duration<int,std::milli> millisecs_t;
    millisecs_t duration(std::chrono::duration_cast<millisecs_t>(end-start));
    std::cout << duration.count() << " milliseconds.\n";
    return 0;
}

When I run the program, it behaves unpredictably. Sometimes, the values A0-9 and B0-9 is printed to console no problem, sometimes there is a segmentation fault with crash report, sometimes, A0-3 & B0-5 is presented.

If i am missing a core synchronization issue, pleasee help

Edit: after alot of useful feed back i changed the code to

#include <iostream>
#include <string>
#include <vector>
#include <mutex>
#include <unistd.h>
#include <thread>
#include <chrono>

using namespace std;
mutex mu;
std::vector<string> ob;

void addSomeAValues(){

for(int a=0; a<10; a++){
    mu.lock();
        ob.push_back("A" + std::to_string(a));
    mu.unlock();
    usleep(300);
}
}

void addSomeBValues(){
for(int b=0; b<10; b++){
    mu.lock();
        ob.push_back("B" + std::to_string(b));
    mu.unlock();

    usleep(300);
}
}

int main() {

std::chrono::steady_clock::time_point    start = std::chrono::steady_clock::now() ;

    thread t0(addSomeAValues);
    thread t1(addSomeBValues);

std::chrono::steady_clock::time_point       end = std::chrono::steady_clock::now() ;

t0.join();
t1.join();

//Display the results
cout << "Code Run Complete; results: \n";
for(auto k : ob){
    cout << k <<endl;
}
//Code running complete, report the time it took
    typedef std::chrono::duration<int,std::milli> millisecs_t ;
    millisecs_t duration( std::chrono::duration_cast<millisecs_t>(end-start) ) ;
    std::cout << duration.count() << " milliseconds.\n" ;
return 0;

}

however I get the following output sometimes:

*** Error in `/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment':
double free or corruption (fasttop): 0x00007f19fc000920 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x80a46)[0x7f1a0687da46]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x402dd4]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x402930]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x402a8d]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x402637
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x402278]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x4019cf]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x4041e3]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x404133]
/home/soliduscode/eclipse_workspace/CppExperiment/Debug/CppExperiment[0x404088]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(+0xb29f0)[0x7f1a06e8d9f0]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x7f8e)[0x7f1a060c6f8e]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7f1a068f6e1d]

Update & Solution

With the problem I was experiencing (namely: unpredictable executing of the program with intermittent dump of corruption complaints), all was solved by including -lpthread as part of my eclipse build (under project settings).

I am using C++11. It's odd, at least to me, that the program would compile without issuing a complaint that I have not yet linked against pthread.

So to anyone using C++11, std::thread, and linux, make sure you link against pthread otherwise your program runtime will be VERY unpredictable, and buggy.

This code works correctly. The only (irrelevant) issue is that you measure the `end` time before the threads are actually joined (ie. done being executed). Is this your real code? — syam, May 28 '13 at 22:57
The code you pasted, after formatting, looks fine (minus required headers). So I must ask is this your **actual** code, or something **similar** to your **actual** code? — Nik Bougalis, May 28 '13 at 22:58
There's no concurrency at all. Using threads makes no sense. That would fix the problem :) — Hans Passant, May 28 '13 at 23:01
@Hans Passant: Well there is no concurrency because OP tries to avoid it by serializing access to ob :) But looks like chaos always wins one way or the other, doesn't it. In what specific way it wins this time is completely beyond me though. Buggy stl/mutex implementation perhaps ? — AlexK, May 28 '13 at 23:07
What platform are you compiling this on? What's are the command line parameters? — sashang, May 28 '13 at 23:10

Jerry Coffin · Answer 1 · 2013-05-29T02:12:29.803

If you're going to use threads, I'd advise doing the job at least a little differently.

Right now, one thread gets the mutex, does all it's going to do (including sleeping for 3000 microseconds), then quits. Then the other thread does essentially the same thing. This being the case, threads have accomplished essentially nothing positive and a fair amount of negative (synchronization code and such).

Your current code is almost unsafe with respect to exceptions -- if an exception were to be thrown inside one of your thread functions, the mutex wouldn't be unlocked, even though that thread could no longer execute.

Finally, right now, you're exposing a mutex, and leaving it to all code that accesses the associated resource to use the mutex correctly. I'd prefer to centralize the mutex locking so its exception safe, and most of the code can ignore it completely.

// use std::lock_guard, if available.
class lock { 
    mutex &m
public:
    lock(mutex &m) : m(m) { m.lock(); }
    ~lock() { m.unlock(); }
};

class synched_vec { 
    mutex m;
    std::vector<string> data;
public:
    void push_back(std::string const &s) { 
        lock l(m);
        data.push_back(s);
    }
} ob;

void addSomeAValues(){
    for(int a=0; a<10; a++){
        ob.push_back("A" + std::to_string(a));
        usleep(300);    
    }
}

This also means that if (for example) you decide to use a lock-free (or minimal locking) structure in the future, you should only have to modify the synched_vec, not all the rest of the code that uses it. Likewise, by keeping all the mutex handling in one place, it's much easier to get the code right, and if you do find a bug, much easier to ensure you've fixed it (rather than looking through all the client code).

@GManNickG: Thanks -- I'd added a comment about std::lock_guard, but in the middle of posting, something got complete mucked up and I forgot to add that again when I put things back together. — Jerry Coffin, May 29 '13 at 02:28

score 0 · Answer 2 · answered May 28 '13 at 23:14

The code in the question runs without any segmentation faults (with adding headers and replacing the sleep with a sleep for my system).

There are two problems with the code though, that could cause unexpected results:

Each thread locks the mutex during his full execution. This prevents the other thread to run. The two threads are not running in parallel! In your case, you should only lock, when you are accessing the vector.
Your end time point is taken after creating the threads and not after they are done executing. Both threads are done, when they are both joined.

Working compilable code with headers, chrono-sleep and the two errors fixed:

#include <mutex>
#include <string>
#include <vector>
#include <thread>
#include <iostream>


std::mutex mu;
std::vector<std::string> ob;

void addSomeAValues(){
    for(int a=0; a<10; a++){
        mu.lock();
        ob.push_back("A" + std::to_string(a));
        mu.unlock();
        std::this_thread::sleep_for(std::chrono::milliseconds(300));
    }
}

void addSomeBValues(){
    for(int b=0; b<10; b++){            
        mu.lock();
        ob.push_back("B" + std::to_string(b));        
        mu.unlock();
        std::this_thread::sleep_for(std::chrono::milliseconds(300));
    }
}

int main() {    
    std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now();        
    std::thread t0(addSomeAValues);        
    std::thread t1(addSomeBValues);    

    t0.join();
    t1.join();
    std::chrono::steady_clock::time_point end = std::chrono::steady_clock::now();

    //Display the results
    std::cout << "Code Run Complete; results: \n";    
    for(auto k : ob){
        std::cout << k << std::endl;
    }
    //Code running complete, report the time it took
    typedef std::chrono::duration<int,std::milli> millisecs_t;
    millisecs_t duration(std::chrono::duration_cast<millisecs_t>(end-start));
    std::cout << duration.count() << " milliseconds.\n";

    return 0;
}

Why is this piece of C++ code not synchronized

Update & Solution

2 Answers2