Boost.Threads - Thread safe operator=()

From: williamkempf_at_[hidden]
Date: 2001-08-09 10:14:51

>Re: Threads docs updated in CVS
--- In boost_at_y..., "Alexander Terekhov" <terekhov_at_d...> wrote:
> faq.html:
> 
> >>   // For assignment we need to synchronize both objects!
> >>   const counter& operator=(const counter& other)
> >>   {
> >>      boost::mutex::scoped_lock scoped_lock(m_mutex);
> >>      boost::mutex::scoped_lock scoped_lock(other.m_mutex);
> >>      m_value = other.m_value;
> >>      return *this;
> >>   }
> 
> is rather dangerous; potential deadlock for
> 
>     thread A:      thread B:
>     =============  =============
>     cntr1 = cntr2; cntr2 = cntr1;
> 
> or even (with non-recursive default mutexes):
> 
>     thread A:
>     =============
>     cntr1 = cntr1;
> 
> how about something like:
> 
>    const counter& operator=(const counter& other)
>    {
>       if ( this != &other ) {
>          boost::mutex::scoped_lock scoped_lock1(this < &other ? 
m_mutex :
> other.m_mutex);
>          boost::mutex::scoped_lock scoped_lock2(this < &other ?
> other.m_mutex : m_mutex);
>          m_value = other.m_value;
>       }
>       return *this;
>    }
> 
> or (with race condition; might be ok in some cases):
> 
>    const counter& operator=(const counter& other)
>    {
>       if ( this != &other ) {
> 
>          int value;
> 
>          {
>              boost::mutex::scoped_lock scoped_lock(other.m_mutex);
>              value = other.m_value;
>          }
>          {
>              boost::mutex::scoped_lock scoped_lock(m_mutex);
>              m_value = value;
>          }
> 
>       }
>       return *this;
>    }

I'm sorry, I responded to this once saying I didn't think it overly 
important to correct the example code because it just illustrated the 
complexities.  I wasn't really recalling the document correctly and 
after having read the FAQ again I agree that this is a grave 
mistake.  In fact, I'm considering a FAQ discussing how to write a 
thread safe operator=() because of this.

Your first example is a well known method for insuring that mutexes 
are locked in the same order to eliminate the chance of deadlock and 
isn't bad for the FAQ in question.  But as an overall means of 
implementing a safe assignment operator it may be thread safe, but 
isn't exception safe.  The second example is better, but it would be 
better to implement this in terms of the well known swap() 
implementation used for exception safety.

    const counter& operator=(const counter& other)
    {
       // assuming a thread safe copy c-tor.
       counter temp(other);
     
       boost::mutex::scoped_lock lock(m_mutex);
       // assuming an exception safe swap method.
       swap(*this, temp);
    }

In the above example swap() would be a private operation that's 
exception safe but not thread safe.  This provides the fastest 
implementation of operator=().  If you need a public swap() as well 
it would need to use the "this trick" from the first example and then 
call the private non-thread safe swap() (obviously we'd need 
different names).

All of this insures thread safety in terms of insuring invariants 
aren't broken, but as you point out there's a race condition between 
construction of the temporary and swapping it with *this.  The 
question is, is this a serious race condition?  Seems to me this race 
condition is no different than that present in the other methods of 
synchronizing this operation because of the small granularity of the 
locks.  This makes it questionable as to whether or not an assignment 
operator should ever be internally synchronized, though for safety it 
should be.  If anything, to me it argues for never allowing an 
internally synchronized object to be copyable/assignable, but this is 
an awfully limiting approach.

Thoughts or comments?

Bill Kempf