At what point to use unique_lock with shared_mutex?

Question

Typically, when using a "normal" mutex, you would use it as in remove1(). However, now with shared_lock and unique_lock, should you use a shared lock first and the unique lock only when necessary? Note that remove() may not need to unique_lock when the model does not exist.

void remove1(int id) {
    std::unique_lock<std::shared_mutex> lock(mutex_);
    for (auto it = models_.begin(); it != models_.end(); ++it)
        if ((*it)->getId() == id)
        {
            it = models_.erase(it);
            return;
        {
}

void remove2(int id) {
    std::shared_lock<std::shared_mutex> sharedLock(mutex_);
    for (auto it = models_.begin(); it != models_.end(); ++it)
        if ((*it)->getId() == id)
        {
            sharedLock.unlock();
            std::unique_lock<std::shared_mutex> uniqueLock(mutex_);
            models_.erase(it);
            return;
        }
}

Answer 1

sharedLock.unlock();
std::unique_lock<std::shared_mutex> uniqueLock(mutex_);

Just because two operations are individually atomic does not mean that one followed by the other represents an atomic sequence. Once you give up a lock, you give it up. If that mutex guards access to the container, there's nothing preventing the iterator you have from being invalidated.

What you're trying to do is have the inner unique_lock atomically upgrade the outer shared_lock in exclusive mode. That can't be done in C++17 at all.

And of course, you never re-lock the shared_lock before you leave the if block, so after erasing one element, you're in trouble.

And that ignores the fact that whenever you erase an element from either of your loops, you'll skip the next one. The iterator returned by erase points to the next element, and your ++it in the loop header will skip it. This is true of both functions.

But in any case, the overall purpose of shared_mutex is to permit multiple readers but only one modifier. Your entire operation is really a modification operation. It may be conditionally modifying, but atomically it is a modification operation. What you want is for everyone to either see the list as it was before the operation or for everyone to see the list as it was after all matching elements are erased. Nobody should ever see the list while you're modifying it.

So it should use exclusive access.