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

@cache_multi_on_arguments does not respect cache expiration time #128

Closed
sqlalchemy-bot opened this Issue Jul 26, 2018 · 11 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Jul 26, 2018

Migrated issue, originally created by Steven James ()

Any function decorated with @cache_multi_on_arguments calls its refresh function on every call, even if the expiration time is set very long.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 26, 2018

Michael Bayer (zzzeek) wrote:

can you share evidence for this? haven't had time to look but the tests should be covering this (perhaps they are not).

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

in commit d521db7 the logic added to check "hard_invalidated" has the wrong element of the "value" being checked, changed from "ct" to "v". been broken since 2016.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

the issue I can see here would only impact when an explicit call to invalidate(hard=True) is involved. will be fixed momentarily.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

it should usually be raising a TypeError since most kinds of objects aren't comparable to the timestamp number.

@sqlalchemy-bot

This comment has been minimized.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Steven James () wrote:

I can try to work up a minimal example or some test code if you'd like... The value returned by the 'v' key appears to be a number also (0 or 1) so I'm not sure it would raise a TypeError.

(Your linked patch does fix the problem)

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

i would gather you're caching integer values for it to be not failing. the test suite is doing the same thing which is partially why they didn't show this problem.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

Use correct element when checking hard invalidated

Fixed issue in the :meth:.CacheRegion.get_or_create_multi method which
was erroneously considering the cached value as the timestamp field if the
:meth:.CacheRegion.invalidate method had ben used, usually causing a
TypeError to occur, or in less frequent cases an invalid result for
whether or not the cached value was invalid, leading to excessive caching
or regeneration. The issue was a regression caused by an implementation
issue in the pluggable invalidation feature added in 🎫38.

Change-Id: Ibc08ec153a4c05d070661278b64695000cff4cef
Fixes: #128

8e9d965

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Changes by Michael Bayer (zzzeek):

  • changed status to closed
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Steven James () wrote:

I think it was trying to look up a mutex value for each key. Once it got the 0/1, it went ahead and determined that the value for that key needed refreshing. Once I saw the 'v' instead of 'ct' there and looked into the change history I stopped looking further.

Thanks for resolving this so quickly!

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 27, 2018

Michael Bayer (zzzeek) wrote:

0.6.7 is released

@sqlalchemy-bot sqlalchemy-bot added the bug label Nov 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment