query queuing_rw_mutex lock state (test for reader/writer state)

[fschaper]

Hello,

I'd like to query the current state of an queuing_rw_mutex, basically to check if I'm currently dealing with an writer lock or an reader that I would need to upgrade.

For me this is usefull in situations where I would call an internal method which changes shared state. I would like to be able to call this method in situations where I already have an writer lock as well as in those where I only have an reader lock. The method therefore needs to have some means to decide if it needs to upgrade the passed lock or not. If I try to call upgrade_to_writer on an existing writer I will run into an assertion.

Are there any reservations from a design view as to adding some method to the scoped_lock f.ex. "bool scoped_lock::is_writer() const" ?

./regards

Florian

[AJ13]

I would normally release the mutex then acquire it again in that situation... or have a private member that I know has to have a write mutex set to be called... so I would hold the write mutex then call the private member function.

I think holding onto a mutex for too long throughout code will make your life miserable in debugging. I think a better approach would be to use the scoped_lock only when required, and get rid of it otherwise... then get it again if it's needed again. This gives a good chance to other things to get a mutex too...

Scott Meyer's had an analogy that might be useful here too... that memory is a toxic chemical that is very useful, but very dangerous. In this analogy, I "touch" mutexed memory as much as required but never any more... I only access mutexed objects for brief periods of time. Same with dealing with toxic chemicals, you don't want to handle them more than required, just enough to get what you need done done.

[fschaper]

I agree with you that a mutex should only be held for the shortest possible amount of time, but then those methods I talk about help me avoid code duplication and are mostly inline besides. In some (but not all) circumstances I could release the mutex but would then have to double-check the original condition for calling the method in the method to call, which might not be known in that context.

p.s. interesting quote.

[AJ13]

Certainly no need for code bloat. Why not use a private member function that is like:

unmutexed_some_operation() ?

You can declare it inline as well if you wish.

AJ

[fschaper]

That would of course be possible but I liked the idea of encapsuling the code that actually needs the writer and the code that guarantees that we are dealing with an writer in one method without having "you have to have a write lock at this point" as a pre-condition in the code. In the end I could not even assert the correct state of any lock because the lock does not expose it's state ,)

[Alexey-Kukanov]

I believe you should know whether the lock is held for reading or for writing and the moment you call the method. So may I suggest you the following idea? Templatize the method with a bool, and use this template parameter inside the method to make necessary decisions:

template
my_method( tbb::queuing_rw_mutex::scoped_lock& mylock )
{
    ....
 if( ! LockedForWrite )
 if( ! mylock.upgrade_to_writer() ) {
 // another thread upgraded first
            // and may have changed the data!
 }
 ....
}

A few things to be noted:

1. The actual template parameter should be a constant, not a variable, for the method to be instantiated. E.g. call it the following way:

   my_method(my_scoped_lock); // called from a reader

2. When compiler instantiates your method, it will perform the check at compilation time and only instantiate the necessary code.
3. You should always check the result of upgrade_to_writer(). If false is returned, it means another thread was first to upgrade and the lock-protected data could have been changed, so you should better re-read those.

[AJ13]

Alexey,

I think what this person wanted was a way to query an existing mutex to determine whether or not it currently had a read lock.

I've wondered about the return value of upgrade_to_writer(), but it was not documented as well as you just outlined here. Perhaps the docs can be updated?

[Alexey-Kukanov]

Adrien,

I understand what the person wanted. I just assumed that it should be known if the mutex is held for reading or writing at the moment the method is called, and suggested an efficient way to pass this information to the method. I doubt it is a good contract between caller and callee if it ensures the lock is held but does not say if it is held for reading or for writing.

As for the docs, it is said in the Reference that the upgrade_to_writer returns "false if lock was released and reacquired; true otherwise". What I added to this is just common sense in the words that do not fit to the format of the Reference. Though I will think about some rewording.

[AJ13]

Thanks for providing the pattern, I did something similar before but not as elegant as I used boost::int_... your method is better.

I agree that this decision should be made at compile time. Is there harm in exposing a bool to tell if a lock is held or not? Or a bool isWriteSafe() that is present only in the RW concept interface? This is not to argue, just to understand the reasoning of the design ;-)

I found that I had to puzzle over the meaning in the docs, and I understood them in a different way... that the bool returned was just an informational message. Thanks for clarifying.

[Alexey-Kukanov]

aj.guillon@gmail.com:

I agree that this decision should be made at compile time. Is there harm in exposing a bool to tell if a lock is held or not? Or a bool isWriteSafe() that is present only in the RW concept interface? This is not to argue, just to understand the reasoning of the design ;-)

The decision can only be made at compile time if the lock state is explicitly provided by a user. If it is requested viaa method or a data member, a compiler can't judge of its (return) value and thus the decision is made at run time.

The harm to provide a method for requesting lock state is as usual with providing an additional method: it extends the interface (and should be supported) and it adds to learning curve. We believe all such extensions should be justified enough to cover additional efforts. In this particular case, the extension also impels to design decision that I would call questionable (if not bad) so there should be really serious reasons to consider. So far, I do not see such reasons.

[AJ13]

I agree with you, better design can be implemented using templates, thanks for the explanation.

[fschaper]

Thank you both for your suggestions, I meanwhile wrapped the lock to solve the problem which should be fine for now.

I understand that you want to keep the interface slim.
But would it be an option to not make upgrade_to_writer on an lock which is already in the active_writer state an error condition?

The documentation states that calling the method on a lock which does not
already hold a reader lock is undefined, which is perfectly fine by me.
If you try to call the method on a lock which is currently in the active_writer state however, one will run into an assertion. If one additional check would be introduced to the method (ok, which would mean some overhead) to see if the lock is already in the requested state and simply return without doing anything, my problem would be fixed without changing the interface. If not, maybe the reference manual could be extended to document this behaviour.

Thanks to both of you for taking the time.

Florian

[fschaper]

MADakukanov:

I believe you should know whether the lock is held for reading or for writing and the moment you call the method. So may I suggest you the following idea? Templatize the method with a bool, and use this template parameter inside the method to make necessary decisions:

[...]

thanks for the suggestion. That could be an compromise. But I still think that a clean solution (in my case) would warrant the object to expose what kind of animal it is, since I would still rely on the vigilant Programmer to remember to change the method call if he changed the lock some place else.

MADakukanov:

[...]

1. You should always check the result of upgrade_to_writer(). If false is returned, it means another thread was first to upgrade and the lock-protected data could have been changed, so you should better re-read those.
   

[...]

thanks for the tip, I'll make sure of that.