rls: fix a data race involving the LRU cache #5925
Conversation
balancer/rls/balancer.go
Outdated
| // cacheMu guards access to the data cache and pending requests map. | ||
| cacheMu sync.RWMutex | ||
| // cacheMu guards access to the data cache and pending requests map. We | ||
| // cannot use an RWMutex here since even a operation like |
balancer/rls/balancer.go
Outdated
| resizeCache := false | ||
| if newCfg.cacheSizeBytes != b.lbCfg.cacheSizeBytes { | ||
| resizeCache = true | ||
| } |
There was a problem hiding this comment.
// Resize the cache if the size in the config has changed.
resizeCache := newCfg.cacheSizeBytes != b.lbCfg.cacheSizeBytes(Comment optional, but explains what could be seen as "more confusing logic", and still uses fewer lines.)
balancer/rls/balancer.go
Outdated
| @@ -284,6 +286,14 @@ func (b *rlsBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error | |||
| b.updateCh.Put(resumePickerUpdates{done: done}) | |||
| b.stateMu.Unlock() | |||
| <-done | |||
|
|
|||
| if resizeCache { | |||
| // If the new config changes the size of the data cache, we might have to | |||
There was a problem hiding this comment.
"If the new config reduces the size"...?
| @@ -284,6 +286,14 @@ func (b *rlsBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error | |||
| b.updateCh.Put(resumePickerUpdates{done: done}) | |||
| b.stateMu.Unlock() | |||
| <-done | |||
|
|
|||
| if resizeCache { | |||
There was a problem hiding this comment.
Why not do this operation above, where we compute it, and remove the bool entirely?
There was a problem hiding this comment.
cacheMu needs to be grabbed before stateMu, which is why we can't do this operation up there. Added a comment for the same.
| // to write to the cache (if we have to send out an RLS request), we will | ||
| // release the read-lock and acquire a write-lock. | ||
| p.lb.cacheMu.RLock() | ||
| p.lb.cacheMu.Lock() |
There was a problem hiding this comment.
Can we reduce the amount of time we're holding this at all?
There was a problem hiding this comment.
The following actions are happening under the lock in this method:
- read the cache entry:
- this is non-blocking and should be fast.
- send an RLS request if required:
- this is the only thing which could block. But the control channel implementation spawns a goroutine to do the actual work of sending the RLS rpc. So, this should also be fast.
- queue the pick or delegate to child policies (or default policy):
- this should also be non-blocking and should be fast.
So, I think we are OK to hold the lock for the whole duration of this method. If we are blocking or taking too long in this method, we have other problems as well I guess (since lb policies are expected to be quick in Pick() and non block in there).
What do you think?
There was a problem hiding this comment.
Seems fine, I was just wondering because the defer makes it impossible to give it up early on one path vs. another, and ideally we hold this for as short a time as possible.
|
Thanks for the quick review !! |
Summary of changes:
sync.RWMutex. In the RPC path, the cache lookup was done under a read-lock, while most of the other operations were done under a write-lock. This was under the assumption that a cache lookup was a read operation. But as it turns out, the cache lookup modifies the LRU cache because it makes the matched entry the most-recently-used.sync.RWMutexand moving to a regularsync.Mutex.iterateAndRunmethod on thedataCachetype.onEvictfield in thecacheEntrytype. This was not being used.This takes care of a P1 internal issue.
RELEASE NOTES: