balancergroup: do not cache closed sub-balancers by default

[easwars]

Internal tracking issue: b/291228739

This PR changes the behavior of the balancergroup package to only cache deleted sub-balancers when a cache timeout is specified explicitly by the caller. This caching feature was supposed to be only used by the priority balancer, but inadvertently has been used by other LB policies that manage child policies.

Fixes #6515

RELEASE NOTES:

• balancergroup: do not cache closed sub-balancers by default; affects rls, weightedtarget and clustermanager LB policies

[dfawley]

Do you think this should go into the cache package for other places to be able to share it? And make the constructor for a timeout cache return it if the timeout is zero?

[easwars]

I thought about this for a while. I feel that it does not make a whole lot of sense for a type that provides caching functionality to also provide a way to disable the same.

• I feel that doing so would unnecessary add more complexity to the package providing the cache.
  • We would need to define an interface and create multiple implementations of it.
• Also, I don't see other usages of this cache where caching might have to be disabled.
  • In fact, we could consider making the cache.NewTimeoutCache return an error if passed a timeout of 0.

Instead explicitly handling the no-caching case in the balancergroup makes more sense because it makes things very clear to the reader of the code as to how and where caching is being done.

[dfawley]

I'm good with the current version. I think if there is a nop implementation then it makes the most sense in the cache package. But just not calling the cache when it's disabled is a perfectly reasonable implementation too.

[dfawley]

Executing this callback synchronously could theoretically cause problems with a pattern like:

b.Lock()
b.cache.Add(blah, blah, func() { b.Lock(); do something; b.Unlock(); })
b.Unlock()

I wonder if we should just go cb() now while seeing this, or wait until we run into that problem, which may never happen anyway.

[easwars]

I agree go cb() should have been what the noop implementation should have done. The problem probably doesn't happen with the current implementation of the timeout cache because the timer callback is invoked in a separate goroutine even if the timeout value is 0.