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@wanadoo.fr> 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.

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