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
Concurrency problem #135
Comments
I am also having this issue occasionally on a small percentage of requests. This affects usage of both the |
I am having the same issue. The code contains a race condition. Our problem seems to be conflicting with the legitimate desire to have A possible fix that would maintain backward compatibility could be to introduce a new keyword argument to If kwargs is EDIT: Typo/syntax |
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 |
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. |
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 Both Redis and Memcached return 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. |
Any updates on this? |
Hi @gergelypolonkai !
I think the right way to check whether key exist or not is to use |
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 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 |
I'm happy to review a PR if someone wants to submit one. I am quite short on time unfortunately... |
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 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. |
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. |
Race condition that leads to None returned value: pallets-eco/flask-caching#135
Race condition that leads to None returned value: pallets-eco/flask-caching#135
Thank you. By the way, the downgrade fixed my problem too! |
It seems that this is just a fix for the |
@mt3925 Good point! Just added PR with the same fix for the |
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.
The text was updated successfully, but these errors were encountered: