-
Notifications
You must be signed in to change notification settings - Fork 156
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
Avoid empty set on multi_cached #490
base: master
Are you sure you want to change the base?
Conversation
The multi_cached decorator was always writing in the cache the result of the function, even if it was empty. This was causing the following error inside aioredis: ERROR aiocache.decorators:decorators.py:370 Couldn't set {}, unexpected error Traceback (most recent call last): File "/Users/josepcugat/workspace/aiocache/aiocache/decorators.py", line 367, in set_in_cache ttl=self.ttl, File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 61, in _enabled return await func(*args, **kwargs) File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 45, in _timeout return await asyncio.wait_for(func(self, *args, **kwargs), timeout) File "/Users/josepcugat/.pyenv/versions/3.6.8/lib/python3.6/asyncio/tasks.py", line 358, in wait_for return fut.result() File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 75, in _plugins ret = await func(self, *args, **kwargs) File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 300, in multi_set await self._multi_set(tmp_pairs, ttl=self._get_ttl(ttl), _conn=_conn) File "/Users/josepcugat/workspace/aiocache/aiocache/backends/redis.py", line 24, in wrapper return await func(self, *args, _conn=_conn, **kwargs) File "/Users/josepcugat/workspace/aiocache/aiocache/backends/redis.py", line 140, in _multi_set await _conn.mset(*flattened) File "/Users/josepcugat/.pyenv/versions/3.6.8/envs/aiocache/lib/python3.6/site-packages/aioredis/util.py", line 52, in wait_ok res = await fut aioredis.errors.ReplyError: ERR wrong number of arguments for 'mset' command
Codecov Report
@@ Coverage Diff @@
## master #490 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 11 11
Lines 950 950
Branches 105 105
=======================================
Hits 945 945
Misses 5 5
Continue to review full report at Codecov.
|
@@ -328,7 +328,7 @@ async def decorator( | |||
result = await f(*new_args, **kwargs) | |||
result.update(partial) | |||
|
|||
if cache_write: | |||
if result and cache_write: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user... with this (not that before it worked as expected) it means that the execution will be executed every time the results are empty, is that the behavior you would expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, in my case I already had something like:
if not params:
return {}
So it wouldn't make any difference. But I can understand that in some cases you might want to do some logic even if the params are empty because everything is cached already.
I would be against putting a new configuration to control this (too many already), so there are 2 options:
- Always call the method even if the params are empty. If you don't want to do anything a shortcut can be added at the beginning of the method to just return, so the behaviour can be controlled.
- Never call the method when the params are empty. In this case if I wanted to do something in the method (like logging) I would not have any workaround.
I would go for option 1 then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks similar to #452. Maybe the 3rd option is what @paulo-raca mentions in the issue of having an "empty" token and write this into Redis (and others). Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would work too, since in that case we would be writing the "guard" value internally instead of the empty dict that causes the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that discussion relates more to the other decorators, where the expected return value may be None
.
For multi_cached though, we should clearly define what an empty dict signifies. I suspect this change is fine and it can be treated as a 'no-cache' for the result, indicating that something failed temporarily and a result couldn't be retrieved. Alternatively, it could simply be an error and a function is expected to always return values for the keys requested. Or it could be treated as a normal result that should be cached.
I'm not sure what the use cases are for this though.
This comment has been minimized.
This comment has been minimized.
If you're still interested in getting this merged, we'll need this updated with latest master. I suspect the test could also be moved into tests/ut/test_decorators.py and done with a mock_cache or similar. |
The multi_cached decorator was always writing in the cache the result of the function, even if it was empty. This was causing the following error inside aioredis: