[thread] is this reverse_lock class a source of errors?
*/ , was_locked_(false) { #ifndef BOOST_SYNC_REVERSE_LOCK_DONT_CHECK_OWNERSHIP /*< Define BOOST_SYNC_REVERSE_LOCK_DONT_CHECK_OWNERSHIP if you don't want to check locker ownership >*/ if (tmp_locker_.mutex()==0) { locker_=tmp_locker_.move(); /*< Rollback for coherency purposes */ throw lock_error(); } #endif if (tmp_locker_) { /*< ensures it is unlocked >*/ tmp_locker_.unlock(); was_locked_=true; } } ~reverse_lock() { if (was_locked) { tmp_locker_.lock(); } locker_=tmp_locker_.move(); /*< Move ownership to nesting locker >*/ }
Hi, Consider the following situations: { unique_lock<mutex> lock(smtx); // ... some writing operations { // non locked block reverse_lock< unique_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again // ... } or { shared_lock<mutex> lock(smtx); // ... some reading operations { // non locked block reverse_lock< shared_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again // ... } Do you think this usage is souhaitable or is this source of errors? The class reverse_lock can be defined as follows: template <typename Locker> class reverse_lock { BOOST_CONCEPT_ASSERT((MovableLockConcept<Locker>)); public: reverse_lock(Locker& locker) : locker_(locker) /*< Store reference to locker >*/ , tmp_locker_(locker.move()) /*< Move ownership to temporaty locker private: Locker& locker_; Locker tmp_locker_; bool was_locked_; reverse_lock(); }; Comments? _____________________ Vicente Juan Botet Escriba
vicente.botet wrote:
{ unique_lock<mutex> lock(smtx);
// ... some writing operations
{ // non locked block reverse_lock< unique_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again
// ... }
I wrote something like this to use inside my condition variable implementation and called it "unlock". I think I prefer the name "unlock" to "reverse_lock", personally. In my case I have only a subset of the proposed C++0x mutex and lock functionality, making the implementation simpler and the potential for errors low. template <typename MUTEX_T> struct Unlock: boost::noncopyable { typedef MUTEX_T mutex_t; mutex_t& m; explicit Unlock(mutex_t& m_): m(m_) { m.unlock(); } ~Unlock() { m.lock(); } mutex_t* mutex(void) { return &m; } }; Once you have recursive mutexes, shared locks and the other features, the possible interactions start to get interesting. I have wondered whether a subset of the C++0x features, closer to the old Boost.Threads API, could be useful as a "threads-for-beginners" package. Phil.
Hi Phil, ----- Original Message ----- From: "Phil Endecott" <spam_from_boost_dev@chezphil.org> To: <boost@lists.boost.org> Sent: Friday, May 09, 2008 3:05 PM Subject: Re: [boost] [thread] is this reverse_lock class a source of errors?
vicente.botet wrote:
{ unique_lock<mutex> lock(smtx);
// ... some writing operations
{ // non locked block reverse_lock< unique_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again
// ... }
I wrote something like this to use inside my condition variable implementation and called it "unlock". I think I prefer the name "unlock" to "reverse_lock", personally. In my case I have only a subset of the proposed C++0x mutex and lock functionality, making the implementation simpler and the potential for errors low.
template <typename MUTEX_T> struct Unlock: boost::noncopyable {
typedef MUTEX_T mutex_t;
mutex_t& m;
explicit Unlock(mutex_t& m_): m(m_) { m.unlock(); }
~Unlock() { m.lock(); }
mutex_t* mutex(void) { return &m; } };
Thanks for the explict contructor and the boost::noncopyable. I have started implementing it like you, but the interaction with the nesting locks is not so clean (nesting_lock has ownership on mtx (1) which is not correct). Of course this class is clean if there is not a nesting lock and much more efficient. { unique_lock<mutex> nesting_lock(mtx); // ... some writing operations { // non locked block reverse_lock< mutex > rlock(mtx); // (1) // ... some code not needing the mutex to be locked } // locked again // ... } We can use the Operator Traits/Concept Traits library (Not yet submitet to Boost) and enable_if to create two partial specialization and have both classes with the same name. I'm not sure that preserve the name is important, but I can, if nobody has already do it, define the BCCL Lockable Concepts and its concept traits counterparts. Currently there are no concepts for the Boost.Thread locks, which seams quite natural because there is neither a class nor a function which takes a Lock as template parameter (except condition_variable_any::wait). Has someone already defined the locks concepts?
Once you have recursive mutexes, shared locks and the other features, the possible interactions start to get interesting.
The interaction with shared_lock seams to be OK. I don't see any problems with recursive_mutex. Could you show, if you have already identified, which interactions can be an issue?
I have wondered whether a subset of the C++0x features, closer to the old Boost.Threads API, could be useful as a "threads-for-beginners" package.
Could you be more precise, to which subset are you referring? Best Vicente
Phil Endecott-48 wrote:
I wrote something like this to use inside my condition variable implementation and called it "unlock". I think I prefer the name "unlock" to "reverse_lock", personally.
unlock_guard would be even better, IMO. I would love if all "call pairs" in boost had supplied RAII wrappers. When I write my own code I usually force client code to use RAII objects. For instance, I've written my own async signal and slots implementation. It consists of two classes AsyncSignal<> and AsyncSignalConnection. These classes are supplied together and the _only_ way to listen to an AsyncSignal is to create an AsynSignalConnection. If the user really needs to call just disconnect earlier on some runtime condition, they can use scoped_ptr+reset. AsyncSignal<void()> sig; boost::scoped_ptr<AsyncSignalConnection> con(new AsyncSignalConnection(sig, foo_call, ...)); if (...) con.reset(); // disconnect Best Regards, Johan -- View this message in context: http://www.nabble.com/-thread--is-this--reverse_lock-class-a-source-of-error... Sent from the Boost - Dev mailing list archive at Nabble.com.
"vicente.botet" <vicente.botet@wanadoo.fr> writes:
Hi,
Consider the following situations:
{ unique_lock<mutex> lock(smtx);
// ... some writing operations
{ // non locked block reverse_lock< unique_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again
// ... }
Do you think this usage is souhaitable or is this source of errors?
I've had to do it myself a few times. I think it's worthwhile adding it to the library in order to ensure it is done as safely as possible. Anthony -- Anthony Williams | Just Software Solutions Ltd Custom Software Development | http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL
Yes, this functionality would be great! I use this pattern quite often (e.g. to not hold the lock while performing a callback) and it's not ideal to do it manually.
--------------------------- Vicente Juan Botet Escriba ----- Original Message ----- From: "Anthony Williams" <anthony_w.geo@yahoo.com> To: <boost@lists.boost.org> Sent: Sunday, May 11, 2008 11:58 AM Subject: Re: [boost] [thread] is this reverse_lock class a source of errors?
"vicente.botet" <vicente.botet@wanadoo.fr> writes:
Hi,
Consider the following situations:
{ unique_lock<mutex> lock(smtx);
// ... some writing operations
{ // non locked block reverse_lock< unique_lock<mutex> > rlock(lock); // ... some code not needing the mutex to be locked } // locked again
// ... }
Do you think this usage is souhaitable or is this source of errors?
I've had to do it myself a few times. I think it's worthwhile adding it to the library in order to ensure it is done as safely as possible.
Anthony
Great, I'm sure you will get a highly-polished implementation. Cheers, Vicente
participants (5)
-
Alex MDC -
Anthony Williams -
Johan Torp -
Phil Endecott -
vicente.botet