cache_on_arguements get does not respect expiration time

[airstandley]

The get method attached by cache_on_arguements does not respect the expiration_time passed to cache_on_arguments.

This leads to potentially bad behavior that decorated_method.get()does not return NO_VALUE when the cached value is older than expiration_time, and the value it does return is different from the value that would have been retrieved by decorated_method(). I can see this leading to quite tricky to solve bugs if the two ways of accessing the cache were used across a codebase.

If this is unintended behaviour then I would suggest decorated_method.get pass expiration_time to self.get. Potentially it may also be wise to add a parameter to cache_on_arguements to ignore expiration_time on get since that is a reasonable use case.
If intended it should be explicitly stated in the documentation to avoid confusion.

Example:

import datetime
from time import sleep
from dogpile.cache import make_region

my_region = make_region().configure('dogpile.cache.memory')

@my_region.cache_on_arguments(expiration_time=1)
def get_cached_datetime():
    return datetime.datetime.now()

get_cached_datetime()  # Returns current time (DateTime 1)
sleep(1)
get_cached_datetime.get() # Returns DateTime1 even though cache value should be invalidated
get_cached_datetime() # Returns current time (DateTime 2)

[zzzeek]

Hi there -

this question is difficult to address because you have not provided any code examples illustrating what you are trying to do and what you expect.

The get method attached by cache_on_arguements does not respect the expiration_time passed to cache_on_arguments.

I guess you are referring to the get() that's added to the decorated function itself? this is correct, the methods attached to the decorated function are only intended to serve as a means of generating the cache key for the function argument which is the hard part.

This leads to potentially bad behavior that decorated_method.get()does not return NO_VALUE when the cached value is older than expiration_time, and the value it does return is different from the value that would have been retrieved by decorated_method().

I don't know that the behavior is "bad" or not, you can pass expiration_time to get() as well. This get() was added by a contributor about five years ago and had no intention of being anything special.

If this is unintended behaviour then I would suggest decorated_method.get pass expiration_time to self.get.

this is unfortunately not possible because it would be backwards incompatible. There's not really a way to solve this problem unless an all new method were added, or perhaps some keyword to the cache_on_arguments that indictes to propagate this to get().

Potentially it may also be wise to add a parameter to cache_on_arguements to ignore expiration_time on get since that is a reasonable use case.

again it would have to be the other way around due to backwards compatibility.

If intended it should be explicitly stated in the documentation to avoid confusion.

that's a good idea, I never thought that the expiration time should be propagated. I'm not sure which of get() or expiration_time were added first to this method.

[airstandley]

Sorry for the lack of code examples. I will try to update later.

I'm not sure what dogpile's official stance is on backwards compatibility, but my two cents is that major versions are typically not backwards compatible for a reason. If you refuse to ever make backwards incompatible changes, it just hurts the project in the end; look at all the headaches Windows has had from Microsoft taking that stance.

[zzzeek]

Sorry for the lack of code examples. I will try to update later.

I'm not sure what dogpile's official stance is on backwards compatibility, but my two cents is that major versions are typically not backwards compatible for a reason. If you refuse to ever make backwards incompatible changes, it just hurts the project in the end; look at all the headaches Windows has had from Microsoft taking that stance.

Hi , and thanks for your comments.

I'm also the author / maintainer of SQLAlchemy, so I have many years of experience both hoisting backwards incompatible changes on users, as well as bending over backwards to not make backwards compatible changes. I think the big thing about a breaking change is that it has to be worth it and it has to break very explicitly, that is, if folks upgrade to the new version and they don't explicitly attend to this API change, their app breaks, which also means, we can instead provide a deprecation path along the way so that when a user upgrades and they don't attend to this change, they get warnings that tell them they should attend to the change.

In this case I don't yet see a way the change can be explicitly breaking or have a deprecation path as proposed. It can only be a subtle behavioral change that applications won't notice and might cause their apps to fail for unknown reasons. dogpile.cache is the kind of library that's buried deep between other layers and is not that noticeable, so I am extremely cautious not to release subtle behavioral changes that are very difficult to notice or warn about, major version or not.

[zzzeek]

this is also why I'm usually extremely cautious about accepting new features too, so certainly, I screwed up not thinking of this use case when .get() was added.

[zzzeek]

plus, why is "expiration_time" optionally a callable only on this function, and not anywhere else like get() etc., and when it is a callable, why does it take no arguments, what if the callable wants to be based on the arguments being passed. the API is all over the place kind of mediocre.

[airstandley]

Hey Mike,
Thanks for the insightful and detailed response. I really appreciate you taking the time to school my naive butt :p

Given the state of 'cache_on_arguements' with all it inconsistencies, what would you think about adding a new decorator method and moving this one towards being deprecated? It would be backward compatible until the old method is removed, and it's an obvious breaking change when the old method is removed. The obvious downsize is it bloats the interface and could be confusing for new users.

Interested in your thoughts on if cleaning this up is worth that cost?

[zzzeek]

adding a new decorator is an option but it also has to take into account cache_multi_on_arguments too. as far as the "expiration_time" is a callable that's a change to other parts of region. I'd be very concerned that if we add a new decorator it will still make a lot of mistakes since there's a lot of issues people have had with this decorator and i really don't have regular enough engagement with dogpile.cache to be able to make a nuanced set of decisions on what the optimal API is; that's why the current situation exists. I don't really have much time to put into thinking about dogpile.cache.

[zzzeek]

i would think that some kind of decorator that is more composable using method chaining would allow it to be more open ended, something like:

@my_region.cache_on_arguments.cache_multi().with_key_generator(my_key_generator)

that might be overkill, but I have an intuition here is that, somehow when expiration_time gained the ability to accept a callable, the architecture of the CacheRegion object should have caused it to accept a callable everywhere. There is something fundamentally not great about the whole thing at this point, but doing something new would be hard and introduce a lot of risk.

also I do agree that the expiration_time being used by my_func.get() is very appropriate and it seems unlikely it would break things very much since I'm not sure that get() method is even used that much. but if it does break something for someone it would be very subtle and I'm not that confident I'm anticipating the impact.

[jvanasco]

sidenote: I think there should be a label dedicated to cache_on_arguments issues.