Add cache wrapper that handles parallel access. #861
Conversation
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
relay/cached_accessor.go
Outdated
| // is written into the channel when it is eventually fetched. If a key is requested more than once while a | ||
| // lookup in progress, the second (and following) requests will wait for the result of the first lookup | ||
| // to be written into the channel. | ||
| lookupsInProgress *sync.Map |
There was a problem hiding this comment.
nit: you don't need to initialize lookupsInProgress if it's typed as sync.Map
There was a problem hiding this comment.
I ended up having to back out this change in favor of using a mutex. The core issue is that reading the lookupsInProgress map needs to be atomic with respect to reading from the cache. Was able to provoke a race condition in a unit test that caused an unnecessary cache miss.
relay/cached_accessor_test.go
Outdated
| // Wait for the goroutines to start. We want to give the goroutines a chance to do naughty things if they want. | ||
| // Eliminating this sleep will not cause the test to fail, but it may cause the test not to exercise the | ||
| // desired race condition. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Sleep is generally not reliable in tests. i.e. I don't think it's guaranteed that it always sleep the right amount in test and it can result in flaky tests.
Maybe we can use something like a buffered channel to wait for all goroutines to trigger?
There was a problem hiding this comment.
In this scenario, I'm not relying on the sleep to make the test pass. The purpose of the sleep statement is to allow background threads a chance to do bad things if they want to do bad things.
In order to show that test stability does not depend on the sleep, I've set this up so that it runs twice: once with the sleep and once without it. Are you ok with this approach? If you'd still prefer to avoid having a sleep, we should chat. One option might just be to remove the sleep entirely.
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Why are these changes needed?
This PR adds a wrapper around
lru.Cache. The purpose of this wrapper is to facilitate efficient lookup if two RPCs request the same data from the relay concurrently. Ideally we only want to fetch the data once in order to serve both requests.Example scenario without cache wrapper:
t=0, client A requests data X. X is not present in the cache, so the relay requests it from S3.t=50, client B requests data X. X is not present in the cache, so the relay requests it from S3.t=200, the request initiated by client A is completed. X is now present in the cache.t=250, the request initiated by client B is completed.Example scenario with cache wrapper:
t=0, client A requests data X. X is not present in the cache, so the relay requests it from S3.t=50, client B requests data X. X is not present in the cache, but since the lookup is already in progress, B's request is paused until A's request is completed.t=200, the request initiated by client A is completed. X is now present in the cache.t=201, the request initiated by client B is wakes up and uses the same data that was used to serve the request from client A.Checks