Mir

Code review comment for lp:~albaguirre/mir/fix-1427976

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

I am OK with the name, perhaps I would slightly prefer BinarySemaphore.

103 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
104 + SemaphoreUnlockIfUnwinding lock{session_lock};1

147 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
148 + SemaphoreUnlockIfUnwinding lock{session_lock};

The current locking scheme (using shared<unique_lock>) is safer. With the new scheme we may never release the lock if something goes wrong with the caller of the functor or the functor itself, since we have dropped RAII.

> I recall semaphore as a type of lock with a counter. The counter is the defining feature
> of a semaphore. And a quick search seems to support this:
> http://en.wikipedia.org/wiki/Semaphore_(programming)

From the bottom of the page:

A mutex is essentially the same thing as a binary semaphore and sometimes uses the
same basic implementation ... Mutexes have a concept of an owner. Only the process
that locked the mutex is supposed to unlock it. If the owner is stored by the mutex
this can be verified at runtime.

"Needs fixing" for the less safe locking scheme, looks good otherwise.

review: Needs Fixing

« Back to merge proposal