Groupcache: Concurrent access to lru.Cache
It doesn't look like lru.Cache is safe for concurrent access. Is this correct? If so, would you be opposed to me adding in a mutex?
I'm new to open source contribution and this looks like it might be an easy win.
abennett
All 3 comments
What would be the downsides of replacing the current map with the new sync.Map?
abennett
on 30 Aug 2017
Hi,
I hope I am not being redundant in responding but I notice that this thread has been silent for a bit.
From looking in groupcache.go, the lru is only used in a thread-safe manner. So the LRU leaves it's locking policy up to the calling code. I think that's reasonable, because then double-locks can be avoided - there's no question of who has to implement the locking, or where it should go in the code.
There are a couple of resources on sync.Map:
https://www.youtube.com/watch?v=C1EtfDnsdDs
https://medium.com/@deckarep/the-new-kid-in-town-gos-sync-map-de24a6bf7c2c
So, I guess the question is: do you have a measured cache contention problem in groupcache? :-)
Cheers,
rbastic
on 22 Oct 2017
Not redundant at all, and your comments make sense. I am satisfied. Thank you!
abennett
on 2 Nov 2017
Related issues
yml
路
3Comments
orcaman
路
9Comments
cowboyrushforth
路
5Comments
AlexanderChen1989
路
6Comments
pquerna
路
3Comments
Most helpful comment
Hi,
I hope I am not being redundant in responding but I notice that this thread has been silent for a bit.
From looking in groupcache.go, the lru is only used in a thread-safe manner. So the LRU leaves it's locking policy up to the calling code. I think that's reasonable, because then double-locks can be avoided - there's no question of who has to implement the locking, or where it should go in the code.
There are a couple of resources on sync.Map:
https://www.youtube.com/watch?v=C1EtfDnsdDs
https://medium.com/@deckarep/the-new-kid-in-town-gos-sync-map-de24a6bf7c2c
So, I guess the question is: do you have a measured cache contention problem in groupcache? :-)
Cheers,