Fix race during concurrent cache entry creation #1053
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1053 +/- ##
==========================================
+ Coverage 46.56% 46.68% +0.12%
==========================================
Files 157 157
Lines 12708 12699 -9
==========================================
+ Hits 5917 5929 +12
+ Misses 6039 6018 -21
Partials 752 752
Continue to review full report at Codecov.
|
arielshaqed
force-pushed
the
bugfix/cache-race
branch
from
45accfc to
72256b8
Compare
When computing a new value to place in the cache, return it from all concurrent GetOrSet calls. Also remove ErrCacheItemNotFound, it can no longer be generated. Cached values (always) can always outlive their lifetimes in the cache itself! The actual values returned from GetOrSet are no longer controlled by the cache.
No longer needed.
It was using cache.ChanLocker, which is now gone.
arielshaqed
force-pushed
the
bugfix/cache-race
branch
from
72256b8 to
a8b1739
Compare
Thanks, golangci, good catch there!
| var e error | ||
| tfs.keyLock.Lock(fileRef.filename, func() { | ||
| _, err := tfs.keyLock.Compute(fileRef.filename, func() (interface{}, error) { | ||
| var err error |
There was a problem hiding this comment.
Alas, yes. The last error handler on l. 264 is nonfatal (not sure why; @itaiad200 can explain) and returned only after being reported to the histograms. So we need to generate an actual err here.
| var e error | ||
| tfs.keyLock.Lock(fileRef.filename, func() { | ||
| _, err := tfs.keyLock.Compute(fileRef.filename, func() (interface{}, error) { | ||
| var err error |
There was a problem hiding this comment.
Alas, yes. The last error handler on l. 264 is nonfatal (not sure why; @itaiad200 can explain) and returned only after being reported to the histograms. So we need to generate an actual err here.
Previously concurrent calls to GetSet on the same cache entry would fail all except one ("the first", for some ordering of the calls). But given that a value returned from GetSet moments after the creation would be good, and that a call moments before creation would shift creation and also work, the lifetime of a value is anyway not determined by the cache. So best to return the newly computed value.
This change is