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().