search

graph-gophers / dataloader

Implementation of Facebook's DataLoader in Golang
MIT License
1.21k stars 75 forks source link

v2: InMemoryCache: fix data race on Clear() method #52

Closed tonyghita closed 5 years ago

tonyghita commented 5 years ago

Fixes #51. Reproduce the issue without this fix by running

$ go test -count=1000 -race .

And verify the fix running the same test command with this commit applied.

Note: #53 should also be merged to resolve all test failures surfaced with that command.

tonyghita commented 5 years ago

Actually, looks like this approach causes issues in v3 where we clear the cache after a batch. I don't think the Range method of a sync.Map is protected, so we can race between Clear() and Get().

tonyghita commented 5 years ago

I changed the approach as mentioned in #51 to use a regular map protected by mutex for correctness.

Using the existing benchmark to compare the sync.Map cache implementation against the map and sync.RWMutex implementation, the map + sync.RWMutex implementation is slightly faster (and correct!).

Closing this PR in favor of one that gets rid of the sync.Map implementation and moves InMemoryCache.go into cache.go.