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

attempt to solve the concurrency issues with get_or_set #179

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PoByBolek
Copy link
Contributor

This should fix issue #149.
When there is no stale value to return, one thread tries to compute func()'s result while all other threads wait for it to complete.

this should fix issue sebleier#149
when there is no stale value to return, one thread tries to compute func()'s result while all other threads wait for it to complete.
@PoByBolek
Copy link
Contributor Author

Apparently not... I can't even reproduce this on my machine :(

…er test case

I *hope* that the build failure at https://travis-ci.org/sebleier/django-redis-cache/jobs/618267821 resulted from thread_2 being too slow so that the key was already expired. Otherwise I can't explain how *both* thread_1 and thread_2 got the values from their value func(). (I would at least have expected that thread_2 came before thread_1 and so they would both get 'c' instead of the expected 'b'...)

Again, I really hope that this was just a bug in the test, not in the actual implementation...
@victorvess
Copy link

victorvess commented Jul 1, 2020

I originally left a comment asking why we wouldn't use Blocking, but I think I get what you're doing here... think I had just a minor tweak I suggested inline with regard to caching of None


if is_fresh:
is_fresh = self._get(client, fresh_key)
if value is not None and is_fresh:

Choose a reason for hiding this comment

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

Suggested change
if value is not None and is_fresh:
if is_fresh:

I suggest maybe we leave the None check out of here because func could explicitly return None here

victorvess added a commit to vndly-oss/django-redis-cache that referenced this pull request Jul 2, 2020
Pulled in fix (with some changes) from sebleier#179
@PoByBolek
Copy link
Contributor Author

I guess I was thinking if get() returns None, then the cached value has expired and that should have precedence over the is_fresh value. Because the Django docs explicitly advise against storing None in caches:

We advise against storing the literal value None in the cache, because you won’t be able to distinguish between your stored None value and a cache miss signified by a return value of None.

So nobody would ever do that, right? ;-)

Moreover, when I do

cache.get_or_set('foo', lambda: 1)
cache.delete('foo')
cache.get_or_set('foo', lambda: 2)

The final get_or_set() would return None with your changes (because the __fresh__foo key wasn't deleted), where I would expect it to return 2 instead.

@victorvess
Copy link

I guess I was thinking if get() returns None, then the cached value has expired and that should have precedence over the is_fresh value. Because the Django docs explicitly advise against storing None in caches:

We advise against storing the literal value None in the cache, because you won’t be able to distinguish between your stored None value and a cache miss signified by a return value of None.

So nobody would ever do that, right? ;-)

Hah, nice I was not aware of this advice that's good to know

Moreover, when I do

cache.get_or_set('foo', lambda: 1)
cache.delete('foo')
cache.get_or_set('foo', lambda: 2)

The final get_or_set() would return None with your changes (because the __fresh__foo key wasn't deleted), where I would expect it to return 2 instead.

https://martinfowler.com/bliki/TwoHardThings.html

Okay makes sense. Yeah I guess this could lead to making delete try to clean up fresh keys too but only get_or_set does those and its another edge case to check and at some point we're back and forth with redis so much are we even saving time caching? 😄

Thanks for the response I'm good with this, now we just need to get your PR accepted cuz this is impacting me too and I would love to not maintain a fork for my company hehe

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.

2 participants