assignment operator for classes having a non copyable boost::mutex

I am reading here this old Boost Thread FAQ where there is a guideline to implement copy-construct开发者_开发问答ion and assignment operator for classes having a boost::mutexnon-copyable object as member.

I am fine with the copy constructor but I have some doubt on the assignment operator. Is still valid the instruction below?

  // old boost thread    
  const counter & operator=( const counter& other ){
     if (this == &other)
        return *this;

     boost::mutex::scoped_lock lock1(&m_mutex < &other.m_mutex ?
                                        m_mutex : other.m_mutex);
     boost::mutex::scoped_lock lock2(&m_mutex > &other.m_mutex ?
                                        m_mutex : other.m_mutex);
     m_value = other.m_value;

     return *this;
}

Should not this be updated to:

 // new boost thread    
 const counter& operator=(const counter& other){
    if (this == &other)
       return *this;

    boost::unique_lock<boost::mutex> l1(m_mutex, boost::defer_lock);
    boost::unique_lock<boost::mutex> l2(other.m_mutex, boost::defer_lock);
    boost::lock(l1,l2);
    m_value = other.m_value;

    return *this;
}

First of all, I assume the question is about avoiding deadlock when locking multiple arbitrary mutexes. The important thing is to always use the same ordering convention throughout the code using a set of mutexes. If you can guarantee that mutex A will always be locked before B, B will always be locked before C, and A always before C, you will avoid deadlock.

In the first code example, the convention is to lock the mutex with the lower memory address first. This will work fine, as address ordering is an invariant. The second version is the official Boost method of avoiding deadlock. The documentation does not specify what ordering is performed internally. I don't recommend looking it up in the source and using this information elsewhere in your code - it might subtly break things if the library changes.

If you're starting from scratch (you've not held more than one mutex at a time in your code before), using Boost's method is definitely preferable - you don't have to worry about the exact ordering as long as you leave it up to boost every time. If the rest of your code uses memory ordering, you must use that. If your code uses some other convention entirely, you need to apply that here, too. Under no circumstances should you mix conventions within any set of locks which may be held at the same time, that's just asking for trouble.

To answer the question in the comment:

A custom lock ordering scheme can be useful in certain circumstances, especially if you need to hold some locks (A) for a long time, but some (B) only briefly, while holding the long one. For example if you need to run long jobs on objects of type A, which briefly affect lots of instances of B. The convention would be to always acquire the lock for A first, then the lock on the B object:

void doStuff(A& a, std::list<B*> bs)
{
  boost::unique_lock<boost::mutex> la(a.mutex); // lock a throughout
  for (std::list<B*>::iterator ib = bs.begin(); ib != bs.end(); ++ib)
  {
    // lock each B only for one loop iteration
    boost::unique_lock<boost::mutex> lb(ib->mutex);
    // work on a and *ib
    // ...
  }
}

You may be able to relinquish the lock on A between each loop iteration and use Boost's/C++0x's lock ordering, but depending on what doStuff() does, that might make the algorithm more complicated or confusing.

Another example: In runtime environments where objects don't necessarily stay in the same memory location (e.g. due to copying garbage collection), relying on the memory address for ordering won't be reliable. You might therefore give each object a unique ID, and base the lock ordering on the ID order.