Concurrency problem

[mt3925]

https://github.com/sh4nks/flask-caching/blob/50bc8af30ae9f85714a119c323e8f09100d7a937/flask_caching/__init__.py#L385-L392

If the cache has expired, after the execution of the code "rv = self.cache.get(cache_key)", there may be other processes that have already written values, but here will return a None instead of calling the function to get the new return value. Sorry for the bad English, all of the above are google translation.

[zendamacf]

I am also having this issue occasionally on a small percentage of requests.

This affects usage of both the memoize & cached decorators.

[lukaskeller]

I am having the same issue.
It seems to be introduced by this recently merged PR.

The code contains a race condition.
If the value for cache_key has been set between get and set by another thread/process, then the None return value could incorrectly appear to be the valid, intended return value.

Our problem seems to be conflicting with the legitimate desire to have None as a valid cache value.

A possible fix that would maintain backward compatibility could be to introduce a new keyword argument to memoize & cached which signals whether None is allowed/intended as a legitimate return value. The default could be True.

If kwargs is True: behaviour as right now with subsequent check on key existence;
else: assume None means that the key doesn't exist and evaluate the base function.

EDIT: Typo/syntax

[gergelypolonkai]

I agree there’s a race condition here, but i’m not sure i’m happy with the solution you (@lukaskeller) propose, as i’m not sure it’s always easy to decide if None is an acceptable cached value. Also, if i decide it’s valid and call cached/memoize indicating this decision, the race condition still remains.

[lukaskeller]

that is true. we can also discuss alternatives that fix the problem.

We could try to use a transactional "check and get" which checks for existence and retrieves the value in an atomic operation, although this will probably require more effort than a surgical change (could also not work with all cache backends) and have a performance penalty for the transaction.

My proposed change would pull the problem away from "return value interpretation" to "contextual interpretation".

Please share your ideas for fixing the race condition, i am happy to look for a solution together.

[gergelypolonkai]

I did a lot of research during the weekend, and it seems the change that brought in this problem is a solution for a problem that never existed (well, it did exist for the simple backend, but nothing else).

Both Redis and Memcached return None if and only if the key to be get does not exist. In fact, there is no way in either backends to store a None value.

As such, it is enough to simply undo the erring PR, and this problem is solved.

If you know about any backends that is able to cache None values, let me know. Otherwise, i will do the required change (hopefully) later today.

[zendamacf]

Any updates on this?

[maxpetriev]

I did a lot of research during the weekend, and it seems the change that brought in this problem is a solution for a problem that never existed (well, it did exist for the simple backend, but nothing else).

Both Redis and Memcached return None if and only if the key to be get does not exist. In fact, there is no way in either backends to store a None value.

As such, it is enough to simply undo the erring PR, and this problem is solved.

If you know about any backends that is able to cache None values, let me know. Otherwise, i will do the required change (hopefully) later today.

Hi @gergelypolonkai !
It seems memcached allows to store None value. I tested this with pylibmc v.1.6.1:

>>> import pylibmc
>>> mc = pylibmc.Client(["127.0.0.1"])
>>> mc['test_key'] = None
>>> del mc['test_key']  # delete existing key works fine 
>>> del mc['non_existent_key']  # delete non existent key cause KeyError
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mpetriev/.local/share/virtualenvs/test-kcH8tj5H/lib/python3.6/site-packages/pylibmc/client.py", line 170, in __delitem__
    raise KeyError(key)
KeyError: 'non_existent_key'

I think the right way to check whether key exist or not is to use __getitem__ instead get with try/except block. KeyError will mean that the key does not exist.

[frankrousseau]

Hello,

I think I have a similar problem. Sometimes the cache returns None, while the cached functions always return something (which leads to errors afterward because a non-None value is expected).
I use the memoize decorator from 1.7.2 version with Redis.

I don't know yet how to fix this problem. Did anyone found a hack to solve this issue?

PS: thank you for this beautiful and useful library

[sh4nks]

I'm happy to review a PR if someone wants to submit one. I am quite short on time unfortunately...

[frankrousseau]

Ok I will dig into the internals of the library and see if I find a solution but I'm short in time too.

[joeportela]

Ok I will dig into the internals of the library and see if I find a solution but I'm short in time too.

You may want to simply revert to version 1.7.1
The problem was introduced in 1.7.2

I'm personally in favor of an argument to the memorize function as proposed above by @lukaskeller. It would require the user to know if their function could return None or not but I think that's a reasonable compromise. The question is just what the default value would be for this option.

[frankrousseau]

Ok thank you, I'm going to revert to 1.7.1 and tell you if the problem still occurs for me.

I will have a look at your solution proposal too @joeportela, but I'm not sure to be the right person to develop it since I don't know the library and the problem well.

[frankrousseau]

Thank you. By the way, the downgrade fixed my problem too!

[mt3925]

It seems that this is just a fix for the memoize function, and the cached function still has this problem. @gergelypolonkai @joeportela

[joeportela]

@mt3925 Good point! Just added PR with the same fix for the cached function