Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Cache): Correct concurrent request coalescing #452

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Apr 10, 2024

Fixes the condition for the swap, which has a race condition that can mean delivering the value we've just literally determined to be too old
Reduce a test timeout to hopefully reduce flakiness
Also fixes an oversight: even if a concurrent request was too old for the maxAge, it can still serve as the base state for an incremental load (not a big deal in the normal case; only the very first load would be suboptimal)

@bartelink bartelink marked this pull request as ready for review April 10, 2024 23:39
@bartelink
Copy link
Collaborator Author

bartelink commented Apr 11, 2024

And yes, the tests should not be using Task.Delay to engineer overlaps and/or expirations. The more correct (and, in retrospect, obvious) approach is to maximise use of TimeProvider and/or events, in order to make them quicker, more deterministic and correct (would really appreciate a PR regarding that; it's unfortunately too far down my overall list of priorities at the moment...)

@bartelink bartelink changed the title fix(Cache): Wait for correct concurrent request fix(Cache): Correct wait for concurrent request Apr 11, 2024
@bartelink bartelink merged commit 6b27ae8 into master Apr 11, 2024
5 checks passed
@bartelink bartelink changed the title fix(Cache): Correct wait for concurrent request fix(Cache): Correct concurrent request coalescing Apr 11, 2024
@bartelink bartelink deleted the cache-concurrency branch April 11, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant