CODEWITHSUNDEEP
c++multithreading
Avihai Marchiano
Avihai Marchiano

Reputation: 3927

Do I need to add a lock on a function that returns a copy of a container?

class Manager {
public:

    list<Employee> getEmployees() {
        // Do I need to lock here?
        return emps_;
    }
    void addEmp(Employee emp); //Here I have lock
private:
    list<Employee> emps_;
};

Instance Manager is shared between multiple threads. Do I need to add lock to the getEmployees member function?

I am pretty sure that i need a lock since the complete list is copied , so any modification that will be done in the meantime (until copy will finished ) can break the copy operation.

I am just asking this because i got few opinion that there is no need to lock.

Edit:

Since its clear now that need to lock , my question is how to do this with minimum overhead. By doing below solution you copy the list twice.:

list<Employee> getEmployees() {
    pthread_mutex_lock( &mutex1 );
    list<Emp> tmp  = emps_; //Copy 1 
    pthread_mutex_unlock( &mutex1 );
    return tmp;//Copy 2
}

Upvotes: 3

Views: 496

Answers (2)

WhozCraig
WhozCraig

Reputation: 66244

The whole list is copied, but the source of the copy may be under modification. stdlib containers support fully-concurrent unlocked read access. Writes on the other hand...

if you're going to be writing to this list when the copy is potentially being made, you need to lock it down. a SWMR (Single Writer Multi Reader) lock is ideal for this, btw, especially if you have a dozens or even hundreds of threads that need to be making copies, with only the occasional write needed. Even then, starvation of the writer-request is a real-concern to be addressed by the implementation of the locking class, but not for the scope of your question here (no pun intended).

Regarding your update, I'm a massive fan of scope-released locks when they are appropriate, and they are so in your case. I.e. a scope-object wrapper around your mutex that latches it on entry, and unlatches it on scope-exit.

I'm quite sure the boost:: guys have such a thing easily accessible, and for that matter C++11 may too (I've not taken that plunge yet due to work schedules; no downtime ugh). But you want something like this:

list<Employee> getEmployees() 
{
    scope_lock latch(&mtx);
    return emps_;
}

The scope_lock above is just a simple class that latches the mutex on construction, and unlatches it on destruction. This protects against the very-real possibility that an exception thrown in your copy-construction will not perma-hang your mutex. Think of it as an exploitation of automatic destruction to unlock a mutex. RAII for the win.

I hope that makes sense. For small get'ers like that such things are ideal. Again, many toolkits including boost will likely have such things built in, and if any boost guys read this please cowboy-up and lend a pointer. The example above is about as trivial a scope lock ideal as you can get.

Finally, for a SWMR, the logic is exactly the same. the only difference is the lock will take an additional argument of whether you're requesting read-or-write access.

Upvotes: 12

Some programmer dude
Some programmer dude

Reputation: 409404

In your case I would say that a lock is not needed, at least not with the code you show us. The reason is that the whole list is copied, so each thread that call the function will get its own private copy of the list.

Upvotes: -3

Related Questions