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

Avoid empty set on multi_cached #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcugat
Copy link
Contributor

@jcugat jcugat commented Apr 22, 2020

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

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
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

Merging #490 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #490   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          11       11           
  Lines         950      950           
  Branches      105      105           
=======================================
  Hits          945      945           
  Misses          5        5           
Impacted Files Coverage Δ
aiocache/decorators.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf675ae...0108a04. Read the comment docs.

@@ -328,7 +328,7 @@ async def decorator(
result = await f(*new_args, **kwargs)
result.update(partial)

if cache_write:
if result and cache_write:
Copy link
Member

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?

Copy link
Contributor Author

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:

  1. 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.
  2. 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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

@CLAassistant

This comment has been minimized.

@Dreamsorcerer
Copy link
Member

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.

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.

4 participants