Re: [Threads-devel] reverse_lock exception safety?

Subject: Re: [Threads-devel] reverse_lock exception safety?
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-12-30 05:51:04

Le 21/12/12 13:58, Fredrik Orderud a écrit :
> On Fri, Dec 21, 2012 at 1:22 PM, Vicente J. Botet Escriba 
> <vicente.botet_at_[hidden] <mailto:vicente.botet_at_[hidden]>> wrote:
>
>     Le 21/12/12 11:14, Fredrik Orderud a écrit :
>>     The implementation of boost::reverse_lock::~reverse_lock calls
>>     "lock" on the mutex retrieved from the provided lock. According
>>     to the documentation [1], "lock" can throw
>>     operation_not_permitted, resource_deadlock_would_occur or
>>     device_or_resource_busy exceptions. I don't see any catching of
>>     exceptions inside the destructor, and am therefore concerned
>>     about the exception safety of reverse_lock.
>>
>>     Could someone please explain if reverse_lock is designed to be
>>     exception safe?
>>
>>     If not, then I would guess that it could be made exception safe
>>     by moving the destructor implementation into an explicit "close"
>>     method, and assert on it being called in the destructor.
>>
>>
>>     [1]
>>     http://www.boost.org/doc/libs/1_52_0/doc/html/thread/synchronization.html#thread.synchronization.mutex_concepts.basic_lockable
>>
>     Let me analyze each possible exception case for boost::mutex:
>
>         *operation_not_permitted*: if the thread does not have the
>         privilege to perform the operation.
>
>     Couldn't we assume that if the thread was able to lock the mutex
>     before it will have the rights?
>
>     *resource_deadlock_would_occur*: if the implementation detects
>     that a deadlock would occur.
>     The current implementation is based on
>
>     [EDEADLK]
>         The current thread already owns the mutex. 
>
>     As the reverse_lock has unlocked it, the thread can not own the
>     mutex, so this error seems not plausible.
>
>     *device_or_resource_busy*: if the mutex is already locked and
>     blocking is not possible.
>
>     I think this is an error on the documentation as lock is based on
>     posix
>
>     "The /pthread_mutex_lock()/ and /pthread_mutex_trylock()/
>     functions will fail if:
>
>     [EINVAL]
>         The /mutex/ was created with the protocol attribute having the
>         value PTHREAD_PRIO_PROTECT and the calling thread's priority
>         is higher than the mutex's current priority ceiling. 
>
>     The /pthread_mutex_trylock()/ function will fail if:
>
>     [EBUSY]
>         The /mutex/ could not be acquired because it was already locked. 
>
>     The /pthread_mutex_lock()/, /pthread_mutex_trylock()/ and
>     /pthread_mutex_unlock()/ functions may fail if:
>
>     [EINVAL]
>         The value specified by /mutex/ does not refer to an
>         initialised mutex object. 
>     [EAGAIN]
>         The mutex could not be acquired because the maximum number of
>         recursive locks for /mutex/ has been exceeded. 
>
>     The /pthread_mutex_lock()/ function may fail if:
>
>     [EDEADLK]
>         The current thread already owns the mutex. 
>
>     The /pthread_mutex_unlock()/ function may fail if:
>
>     [EPERM]
>         The current thread does not own the mutex. 
>
>     These functions will not return an error code of [EINTR]."
>
>     Of course other model of Lockable could have other specificities.
>     The question is, what can we do on the destructor if lock()
>     throws? abort? ignore?
>
>     The current situation call terminate(). Ignoring the error seems
>     not a good approach,  or is it?
>
> You might very well be correct in that, reverse_lock::~reverse_lock in 
> practice never throws when used in conjunction with the current mutex 
> implementations provided with boost. But this is only part of the 
> story. I'm pretty sure that there are boost users (besides me) that 
> uses boost locks in conjunction with independent mutex 
> implementations. And then your only guarantees are that the mutex 
> adheres to the BasicLockable specification.
>
I agree with you that there is an potential issue.

> My thought for safer design would be to move the locking code away 
> from the destructor and to a separate close() method that must be 
> called explicitly. This is a bit awkward, but results in a design 
> where the destructor is guaranteed to never throw. The destructor 
> should then check if close() has been called, and call terminate() if 
> not. Note that this terminate() is semantically different from the 
> current terminate(), since it's caused by a programmer error (forgot 
> to close), compared to runtime-error (locking failure).
What would be the expected behavior if close() is called and throws 
(locking error). E.g

{
   reverse_lock<mutex> rlk(mtx);
   no_throw_fct();
   rlk.close(); // throws
}

Should the rlk destructor call terminate()?
If not, which recoverable action could take the user?
>
> I believe that C++11 std::thread follows a similar pattern in respect 
> to joining that must be done explicitly instead of being done 
> automatically in the destructor.
>
This could be a patter to follow, but I would associate it to a 
different lock class, e.g. check_lock_guard. For reverse_lock I would 
prefer to just document that the Lockable parameter must not throw when 
lock() is called inside the destructor. What do you think?

Best,
Vicente