Actually I don't want to copy a mutex. I just want my class that has a mutex to be copyable, without having to write a copy ctor and operator= (that copy everything except the mutex). Using this class solves the problem for me, but I have a feeling it is not safe to do this. class recursive_mutex_ignorecopy : public boost::recursive_mutex { public: recursive_mutex_ignorecopy() : boost::recursive_mutex() {} ~recursive_mutex_ignorecopy() {} recursive_mutex_ignorecopy(const recursive_mutex_ignorecopy&) {} recursive_mutex_ignorecopy& operator=(const recursive_mutex_ignorecopy&) {} }; Is this a good solution that could maybe be made part of boost, in such a way that it is guaranteed to work and be safe? Or is there a better solution? Thanks, Phillip Hellewell P.S. Perhaps this is a moot point, sinceI just realized that for thread-safety I really need to write my own copy ctor and operator= anyways, that will be synchronized with my other functions that initialize shared pointers and stuff. And I need to lock the rhs's mutex too...
Phillip Hellewell wrote: <---- snip ---->
Is this a good solution that could maybe be made part of boost, in such a way that it is guaranteed to work and be safe?
Or is there a better solution?
How about using boost::shared_ptr. Something like this in your class instead: boost::shared_ptr< boost::recursive_mutex > myMutex; -- ---------------------------------- Michael Caisse Object Modeling Designs www.objectmodelingdesigns.com
Michael Caisse wrote:
Phillip Hellewell wrote:
<---- snip ---->
Is this a good solution that could maybe be made part of boost, in such a way that it is guaranteed to work and be safe?
Or is there a better solution?
How about using boost::shared_ptr. Something like this in your class instead:
boost::shared_ptr< boost::recursive_mutex > myMutex;
Then you would protect both copies of the resource that is part of the class by a single (shared) mutex. That doesn't sound right. Regards, Stefan -- ...ich hab' noch einen Koffer in Berlin...
On 7/20/07, Stefan Seefeld <seefeld@sympatico.ca> wrote:
Michael Caisse wrote:
How about using boost::shared_ptr. Something like this in your class instead:
boost::shared_ptr< boost::recursive_mutex > myMutex;
Then you would protect both copies of the resource that is part of the class by a single (shared) mutex. That doesn't sound right.
Exactly, I don't want to copy or share the mutex in any way. I want each object to have its own mutex. I just wanted to avoid having to write copy ctor for no reason. But I just realized that my whole argument is definitely moot. In all my classes with mutexes that I want to be copyable, I need to write a copy ctor and operator= anyways to make them thread-safe. For instance, I'll have a function, say foo(), that initializes a shared pointer of some kind. This function is already synchronized using scoped_lock(m_mutex). To be thread-safe, I really need my copy ctor to be synchronized (with rhs.m_mutex) and my operator= to be synchronized too (with both m_mutex and rhs.m_mutex). Phillip
Phillip Hellewell wrote:
On 7/20/07, Stefan Seefeld <seefeld@sympatico.ca> wrote:
Michael Caisse wrote:
How about using boost::shared_ptr. Something like this in your class instead: boost::shared_ptr< boost::recursive_mutex > myMutex; Then you would protect both copies of the resource that is part of the class by a single (shared) mutex. That doesn't sound right.
Exactly, I don't want to copy or share the mutex in any way. I want each object to have its own mutex.
Right. And how do you think the compiler will know that ? Regards, Stefan -- ...ich hab' noch einen Koffer in Berlin...
On 7/20/07, Stefan Seefeld <seefeld@sympatico.ca> wrote:
Phillip Hellewell wrote:
Exactly, I don't want to copy or share the mutex in any way. I want each object to have its own mutex.
Right. And how do you think the compiler will know that ?
I was agreeing with you. Using a shared_ptr doesn't work. Every object should be protected with its own mutex. Phillip
Phillip Hellewell wrote:
Exactly, I don't want to copy or share the mutex in any way. I want each object to have its own mutex.
My mistake. I see what you are trying to do now. It might be personal preference (with a mix of seeing a lot of errors in the past) but I prefer to write both the ctor and operator= by hand regardless of it using the default behavior. Several places I have worked required it to get through the code review. It presents an issue for maintenance; however, you know that the engineer has thought about copy and assignment semantics for the class. Copying or assigning objects with multithreaded concepts is almost never as easy as the default... and if it is, I like to see a comment saying so and why. Best Regards - Michael -- ---------------------------------- Michael Caisse Object Modeling Designs www.objectmodelingdesigns.com
On 7/20/07, Michael Caisse <boost@objectmodelingdesigns.com> wrote:
assignment semantics for the class. Copying or assigning objects with multithreaded concepts is almost never as easy as the default... and if it is, I like to see a comment saying so and why.
Agreed. In fact, I was just thinking how grateful I am that many of my classes are non-copyable, non-assignable, and read-only (e.g., all functions are const). That makes it pretty easy to make them thread-safe. I just use shared pointers to "copy" them around. Some of my classes are non-copyable and non-assignable, but may change after being constructed. These aren't too hard to make thread-safe, e.g., by synchronizing the functions that make changes. Making a copyable class thread-safe isn't too bad either. For example, you can synchronize the copy ctor using the rhs's mutex(es). This should prevent getting an inconsistent copy. However, even the simplest _assignable_ class seems almost impossible to me to make thread-safe. You can synchronize operator= using both its own mutex(es) and rhs's mutex(es), which seems to solve the problem. But how do you prevent deadlock from something like this? Thread 1: x = y Thread 2: y = x Thread 2 is going to lock the mutexes in the opposite order of thread 1. Ouch! Oh well, not all my classes need to be thread-safe :) I'll just put a little comment here... Phillip
Phillip Hellewell wrote:
However, even the simplest _assignable_ class seems almost impossible to me to make thread-safe. You can synchronize operator= using both its own mutex(es) and rhs's mutex(es), which seems to solve the problem. But how do you prevent deadlock from something like this?
Thread 1: x = y Thread 2: y = x
One common method is to perform the locks based on ordering of the mutex's memory address. That way all threads are locking and releasing in the same order. Michael -- ---------------------------------- Michael Caisse Object Modeling Designs www.objectmodelingdesigns.com
Phillip Hellewell wrote:
However, even the simplest _assignable_ class seems almost impossible to me to make thread-safe. You can synchronize operator= using both its own mutex(es) and rhs's mutex(es), which seems to solve the problem. But how do you prevent deadlock from something like this?
Thread 1: x = y Thread 2: y = x
Thread 2 is going to lock the mutexes in the opposite order of thread 1. Ouch!
The classic solution is to lock the mutexes in ascending order of their addresses, but I prefer X& X::operator=( X const& rhs ) { X tmp( rhs ); // temporarily locks rhs.mutex scoped_lock lock( this->mutex ); swap( tmp ); // or move_from( tmp ) return *this; } It's always a good idea to only lock one mutex at a time if you can afford it.
On 7/20/07, Peter Dimov <pdimov@pdimov.com> wrote:
The classic solution is to lock the mutexes in ascending order of their addresses, but I prefer
X& X::operator=( X const& rhs ) { X tmp( rhs ); // temporarily locks rhs.mutex
scoped_lock lock( this->mutex ); swap( tmp ); // or move_from( tmp )
return *this; }
It's always a good idea to only lock one mutex at a time if you can afford it.
Very nice! Thanks Michael and Peter. I especially liked your last solution. I guess that shows how an efficient swap function can really come in handy. Phillip
"Peter Dimov" <pdimov@pdimov.com> writes:
Phillip Hellewell wrote:
However, even the simplest _assignable_ class seems almost impossible to me to make thread-safe. You can synchronize operator= using both its own mutex(es) and rhs's mutex(es), which seems to solve the problem. But how do you prevent deadlock from something like this?
Thread 1: x = y Thread 2: y = x
Thread 2 is going to lock the mutexes in the opposite order of thread 1. Ouch!
The classic solution is to lock the mutexes in ascending order of their addresses, but I prefer
X& X::operator=( X const& rhs ) { X tmp( rhs ); // temporarily locks rhs.mutex
scoped_lock lock( this->mutex ); swap( tmp ); // or move_from( tmp )
return *this; }
It's always a good idea to only lock one mutex at a time if you can afford it.
I like the idea, but surely the publicly-visible swap should lock both mutexes? Wouldn't this be better as internal_swap()? Anthony -- Anthony Williams Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL
On Jul 23, 2007, at 3:53 AM, Anthony Williams wrote:
"Peter Dimov" <pdimov@pdimov.com> writes:
Phillip Hellewell wrote:
However, even the simplest _assignable_ class seems almost impossible to me to make thread-safe. You can synchronize operator= using both its own mutex(es) and rhs's mutex(es), which seems to solve the problem. But how do you prevent deadlock from something like this?
Thread 1: x = y Thread 2: y = x
Thread 2 is going to lock the mutexes in the opposite order of thread 1. Ouch!
The classic solution is to lock the mutexes in ascending order of their addresses, but I prefer
X& X::operator=( X const& rhs ) { X tmp( rhs ); // temporarily locks rhs.mutex
scoped_lock lock( this->mutex ); swap( tmp ); // or move_from( tmp )
return *this; }
It's always a good idea to only lock one mutex at a time if you can afford it.
I like the idea, but surely the publicly-visible swap should lock both mutexes? Wouldn't this be better as internal_swap()?
Coming to the party late... The above is inefficient for types that are not cheaply swappable. Here is another, admittedly C++0X suggestion which works both for expensive-to-swap and cheap-to-swap types: X& X::operator=( X const& rhs ) { namespace pstd = proposed_std; pstd::unique_lock<pstd::mutex> this_lock(this->mutex, pstd::defer_lock); pstd::unique_lock<pstd::mutex> that_lock(rhs.mutex, pstd::defer_lock); pstd::lock(this_lock, that_lock); // avoids deadlock // do the assign, both *this and rhs are locked return *this; } The above might be better implemented if there is a rw_mutex available: X& X::operator=( X const& rhs ) { namespace pstd = proposed_std; pstd::unique_lock<pstd::mutex> this_lock(this->mutex, pstd::defer_lock); pstd::shared_lock<pstd::mutex> that_lock(rhs.mutex, pstd::defer_lock); pstd::lock(this_lock, that_lock); // ok with different lock types // do the assign, *this write-locked, rhs is only read-locked return *this; } With variadic templates, one can implement pstd::lock with an arbitrary number of mutexes/locks requiring nothing more than lock(), try_lock() and unlock(). And it has strong exception safety assuming a nothrow unlock(). I.e. when it exits exceptionally (if lock() or try_lock() throws), all locks are unlocked. When it exits normally, all locks are locked, no deadlock guaranteed. And for bonus points, this algorithm blows away the performance of locking in order (which is just dead-slow in the measurements I've made - and don't forget or ignore the yields). The below can be implemented today for two locks by just striking out the variadic stuff. Or the greater than two locks overloads could be added manually (what a pain!). -Howard // returns index of failed lock or -1 on success template <class L1, class L2> int try_lock(L1& l1, L2& l2) { unique_lock<L1> u(l1, try_to_lock); if (u.owns()) { if (l2.try_lock()) { u.release(); return -1; } return 1; } return 0; } template <class L1, class L2, class ...L3> int try_lock(L1& l1, L2& l2, L3& l3...) { unsigned r = 0; unique_lock<L1> u(l1, try_to_lock); if (u.owns()) { r = try_lock(l2, l3...); if (r == -1) u.release(); else ++r; } return r; } template <class L1, class L2> void lock(L1& l1, L2& l2) { while (true) { { unique_lock<L1> u1(l1); if (l2.try_lock()) { u1.release(); break; } } std::this_thread::yield(); { unique_lock<L2> u2(l2); if (l1.try_lock()) { u2.release(); break; } } std::this_thread::yield(); } } namespace detail { // On failed attempts, next try always starts // with lock that failed the try_lock on the // previous iteration (i.e. the algorithm blocks on it). template <class L1, class L2, class ...L3> void lock_first(int i, L1& l1, L2& l2, L3& l3...) { while (true) { switch (i) { case 0: { unique_lock<L1> u1(l1); i = try_lock(l2, l3...); if (i == -1) { u1.release(); return; } } ++i; std::this_thread::yield(); break; case 1: { unique_lock<L2> u2(l2); i = try_lock(l3..., l1); if (i == -1) { u2.release(); return; } } if (i == sizeof...(L3)) i = 0; else i += 2; std::this_thread::yield(); break; default: lock_first(i - 2, l3..., l1, l2); return; } } } } // detail template <class L1, class L2, class ...L3> inline void lock(L1& l1, L2& l2, L3& l3...) { detail::lock_first(0, l1, l2, l3...); }
Michael Caisse wrote:
Several places I have worked required it to get through the code review. It presents an issue for maintenance; however, you know that the engineer has thought about copy and assignment semantics for the class.
And trades one type of potential error for another, which is adding stuff to a class and forgetting to update the copy and assignment operators. -- Jon Biggar Levanta jon@levanta.com 650-403-7252
Phillip Hellewell wrote:
Actually I don't want to copy a mutex. I just want my class that has a mutex to be copyable, without having to write a copy ctor and operator= (that copy everything except the mutex).
It's hard to tell without talking about the semantics. What about your class (instance) do you want to copy ? And what should happen if, at the time you attempt to copy, the mutex is locked ? Regards, Stefan -- ...ich hab' noch einen Koffer in Berlin...
participants (7)
-
Anthony Williams -
Howard Hinnant -
Jonathan Biggar -
Michael Caisse -
Peter Dimov -
Phillip Hellewell -
Stefan Seefeld