Does this singleton instance implementation has a race condition?

Question

Someone told me the memCacheInstance has race condition, but go run -race could not tell.

Codes:

type MemCache struct {
    data []string
}

var memCacheInstance *MemCache
var memCacheCreateMutex sync.Mutex

func GetMemCache() *MemCache {
    if memCacheInstance == nil {
        memCacheCreateMutex.Lock()
        defer memCacheCreateMutex.Unlock()

        if memCacheInstance == nil {
            memCacheInstance = &MemCache{
                data: make([]string, 0),
            }
        }
    }
    return memCacheInstance
}

Answer 1

The go race detector does not detect every race, but when it does, it's always a positive case. You have to write code that simulates the racy behavior.

Your example has data race if GetMemCache() is called from multiple goroutines. This simple example triggers the race detector:

func main() {
    go GetMemCache()
    GetMemCache()
}

Run it with go run -race ., output is:

==================
WARNING: DATA RACE
Read at 0x000000526ac0 by goroutine 6:
  main.GetMemCache()
      /home/icza/gows/src/play/play.go:13 +0x64

Previous write at 0x000000526ac0 by main goroutine:
  main.GetMemCache()
      /home/icza/gows/src/play/play.go:18 +0x17e
  main.main()
      /home/icza/gows/src/play/play.go:28 +0x49

Goroutine 6 (running) created at:
  main.main()
      /home/icza/gows/src/play/play.go:27 +0x44
==================
Found 1 data race(s)
exit status 66

It has a race because the first read of the memCacheInstance variable is without locking, without synchronization. All concurrent accesses to a variable must be synchronized where at least one of the accesses is a write.

A simple fix is to remove the unsynchronized read:

func GetMemCache() *MemCache {
    memCacheCreateMutex.Lock()
    defer memCacheCreateMutex.Unlock()

    if memCacheInstance == nil {
        memCacheInstance = &MemCache{
            data: make([]string, 0),
        }
    }

    return memCacheInstance
}

Also note that to execute some code once only in a concurrency-safe manner, there's sync.Once. You may use it like this:

var (
    memCacheInstance *MemCache
    memCacheOnce     sync.Once
)

func GetMemCache() *MemCache {
    memCacheOnce.Do(func() {
        memCacheInstance = &MemCache{
            data: make([]string, 0),
        }
    })

    return memCacheInstance
}

Also note that if you initialize your variable "right away" (when declared or in a package init() function), there's no need for synchronization (because package initialization runs in a single goroutine):

var memCacheInstance = &MemCache{
    data: make([]string, 0),
}

func GetMemCache() *MemCache {
    return memCacheInstance
}

In which case you may also choose to export the variable and then there's no need for GetMemCache().

Answer 2

This is an answer for people who are more familiar with the Go memory model, if you want to roll your own double-checked lock, or want to understand how things work under the hood.

Let's take a look at your implementation:

func GetMemCache() *MemCache {
    if memCacheInstance == nil { // (1)
        memCacheCreateMutex.Lock() // (2)
        defer memCacheCreateMutex.Unlock() // (3)

        if memCacheInstance == nil { // (4)
            memCacheInstance = &MemCache{
                data: make([]string, 0), // (5)
            } // (6)
        }
    }
    return memCacheInstance // (7)
}

Your implementation is wrong because it does not acquire/release the result properly, as well as does not synchronise the pointer accesses correctly.

The second problem is that (1) can return a non-nil while (7) still observes a nil pointer. This is easier to fix, just read the cache only once outside the lock.

func GetMemCache() *MemCache {
    cached := memCacheInstance // (1)
    if cached != nil {
        return cached
    }
    memCacheCreateMutex.Lock() // (2)
    defer memCacheCreateMutex.Unlock() // (3)

    cached = memCacheInstance // (4)
    if cached == nil {
        cached = &MemCache{
            data: make([]string, 0), // (5)
        }
        memCacheInstance = cached // (6)
    }
    return cached
}

The remaining problem is the acquire/release semantics, that is, you want to make sure that if a thread observes a non-nil in (1), it must also observe the write (5) to initialise the object. This requires constructing a happen-before relationship between the store (6) from a thread and the load (1) from another thread, which can be achieved using sync/atomic. Changing the type of memCacheInstance to atomic.Pointer[MemCache], we have:

func GetMemCache() *MemCache {
    cached := memCacheInstance.Load() // (1)
    if cached != nil {
        return cached
    }
    memCacheCreateMutex.Lock() // (2)
    defer memCacheCreateMutex.Unlock() // (3)

    cached = memCacheInstance.Load() // (4)
    if cached == nil {
        cached = &MemCache{
            data: make([]string, 0), // (5)
        }
        memCacheInstanc.Store(cached) // (6)
    }
    return cached
}

• Now, if (1) observes non-nil, then it happens after (6) in some goroutine, which happens after (5) in that goroutine, so the function returns a pointer to a correctly initialised MemCache.

• Otherwise, (1) observes nil, then it proceeds to lock the mutex.

  • If in this case, (4) observes non-nil, then it must be the case that another goroutine has already entered the lock, populate the cache and leave the lock before, which means that (5), (6) happens before (3) in that goroutine, which happens before (2) in the current goroutine, which in turn happens before (4), and we have (4) observes a pointer to a correctly initialised MemCache.

  • The remaining circumstance is that (4) observes nil, which means that it initialise the object itself, so the returned result is obviously properly initialised.

As a result, every code path leads to the function returns a pointer to a properly initialised MemCache.

Note that the read in (4) actually does not have to be atomic.