Mutex used right?

Question

I'm a bit confused about mutex locken/unlocking more times after another. I'm using a RWMutex and all goroutines will have the same mutex of course.

Is this code still race-protected when using mutexes this often?

func (r *Redis) RedisDb(dbId DatabaseId) *RedisDb {
    r.Mu().RLock()
    size := len(r.redisDbs) // A
    r.Mu().RUnlock()
    if size >= int(dbId) { // B
        r.Mu().RLock()
        db := r.redisDbs[dbId] // C
        r.Mu().RUnlock()
        if db != nil { // D
            return db
        }
    }
    // E     create db...
}

Example situation I would think of can happen:

1. gorountine1 and goroutine2 are running both this function
2. both are at point A so that variable size is 3
3. condition B is true for both goroutines
4. both read C at the same time
5. variable db is nil for both goroutines so condition C is false
6. now both goroutines are going to E and create the same database 2 times, thats bad

Or do I have to lock/unlock all one time in this situation?

func (r *Redis) RedisDb(dbId DatabaseId) *RedisDb {
    r.Mu().Lock()
    defer r.Mu().Unlock()
    size := len(r.redisDbs)
    if size >= int(dbId) {
        db := r.redisDbs[dbId]
        if db != nil {
            return db
        }
    }
    // create db...
}

Solution

func (r *Redis) RedisDb(dbId DatabaseId) *RedisDb {
    getDb := func() *RedisDb { // returns nil if db not exists
        if len(r.redisDbs) >= int(dbId) {
            db := r.redisDbs[dbId]
            if db != nil {
                return db
            }
        }
        return nil
    }

    r.Mu().RLock()
    db := getDb()
    r.Mu().RUnlock()
    if db != nil {
        return db
    }

    // create db
    r.Mu().Lock()
    defer r.Mu().Unlock()
    // check if db does not exists again since
    // multiple "mutex readers" can come to this point
    db = getDb()
    if db != nil {
        return db
    }
    // now really create it
    // ...
}

Answer 1

Welcome to the world of synchronization. Your assessment is correct, there is opportunity for concurrency issues to arise with the first implementation. With the second, those concurrency issues are removed, but it's fully locked, with no opportunities for even concurrent read access. You don't have to do it that way, you could do an initial check with a read lock, then if that check determines creation is needed, establish a write lock, then re-check, create if still necessary, then unlock. This isn't an unusual construct. It's less efficient (due to performing the check twice) so it's up to you to make a decision on the trade-off, mostly based on how expensive the check is to do twice, and how often the function would be able to operate in the read-only path.