luca3m / redis3m

A C++ Redis client
Apache License 2.0
190 stars 78 forks source link

exception safe locks/unlocks #26

Closed ovanes closed 9 years ago

ovanes commented 9 years ago

I just wonder, why sometimes is explicit locking on a mutex object is used instead of a unique_lock with explicit unlock call.

In the file: redis3m/src/connection_pool.cpp or redis3m/src/simple_pool.cpp I code like that:

connection::ptr_t simple_pool::get()
{
    connection::ptr_t ret;

    access_mutex.lock();
    std::set<connection::ptr_t>::iterator it = connections.begin();
    if (it != connections.end())
    {
        ret = *it;
        connections.erase(it);
    }

    access_mutex.unlock();

    if (!ret)
    {
        ret = connection::create(_host, _port);
        // Setup connections selecting db
        if (_database != 0)
        {
            ret->run(command("SELECT") << _database);
        }
    }
    return ret;
}

Why not writing it like:

connection::ptr_t simple_pool::get()
{
    connection::ptr_t ret;

    std::unique_lock<std::mutex> lock(access_mutex);
    std::set<connection::ptr_t>::iterator it = connections.begin();
    if (it != connections.end())
    {
        ret = *it;
        connections.erase(it);
    }

    lock.unlock();

    if (!ret)
    {
        ret = connection::create(_host, _port);
        // Setup connections selecting db
        if (_database != 0)
        {
            ret->run(command("SELECT") << _database);
        }
    }
    return ret;
}

In fact unique_lock is used 30 lines below...

I understand that set::erase(iterator) version has the nothrow guarantee, but still I think consistency would be better. And there is no performance drawback in that case.

luca3m commented 9 years ago

Usually I use explicit lock instead of unique_lock when I need to unlock it before the scope exits. It can be achieved also using unique_lock so I agree with you, to be more consistent.

luca3m commented 9 years ago

27 pull request, that I've just merge should address this