Re: [Threads-devel] reverse_lock exception safety?

Subject: Re: [Threads-devel] reverse_lock exception safety?
From: Fredrik Orderud (forderud_at_[hidden])
Date: 2012-12-21 07:58:58

On Fri, Dec 21, 2012 at 1:22 PM, Vicente J. Botet Escriba <
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.

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).

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.

Regards,
Fredrik Orderud