Boost logo

Boost :

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

Boost list run by bdawes at, gregod at, cpdaniel at, john at