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

Bug - Realtime cache collision #2589

Merged

Conversation

ADBond
Copy link
Contributor

@ADBond ADBond commented Jan 20, 2025

Fixes #2515.

There were occasional failings of test runs in tests/test_realtime.py, particularly in CI, that would generally be resolved on re-running.

The issue was that we used id(settings) as a cache key. In CPython this is simply the address of the object in memory, and in general is not guaranteed to be unique. When new objects are put in the same memory location as those that have been garbage-collected, we got cache collisions, resulting in the wrong SQL being returned.

We use a slightly different approach for a cache key to how we check if we have some SQL already cached, depending on the type of object:

  • pathlib.Path or str (representing saved models) we just use the string itself
  • dict we use a json dump (possibly after converting to serialisable types)
  • for SettingsCreator we use the id still, but we also store a weak reference to the object. This won't prevent it being garbage collected, but allows us to see if the object still lives. If we find a supposèd cache hit, we check the associated weakref (which we also keep in the cache), and if it's dead (and thus we have the case of recycled id values) we delete the entry and return as though we found no hit.

The custom solutions for str and dict are mainly because these cannot be (directly) weakrefed.

You can see in this PR test runs where we see that this weakref solution is triggered, and fixes the issue (with a hack to fail the tests on a separate condition, which occurs only on that logic branch).

@ADBond ADBond added bug Something isn't working caching labels Jan 20, 2025
@ADBond ADBond requested a review from RobinL January 20, 2025 13:24
Copy link
Member

@RobinL RobinL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@ADBond ADBond merged commit 851b73e into moj-analytical-services:master Jan 20, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working caching
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Realtime test sometime failing?
2 participants