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 , "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.
> 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
> 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
> "The /pthread_mutex_lock()/ and /pthread_mutex_trylock()/
> functions will fail if:
> 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:
> 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:
> The value specified by /mutex/ does not refer to an
> initialised mutex object.
> 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:
> The current thread already owns the mutex.
> The /pthread_mutex_unlock()/ function may fail if:
> 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
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?