Skip to content

Fixed Redis connectivity problems number one and two #338

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

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

Conversation

excitoon
Copy link
Contributor

Before that fix, it was not possible to use library when Redis restarts sometimes.

Please merge this PR or previous one (#337) and also backport one of them to all older versions, as the bug renders library impossible to use with Redis in production.

@excitoon
Copy link
Contributor Author

excitoon commented Mar 9, 2025

@Krukov actually this PR is not so good, you shall use redis_connect_func=...:

def redis_connect_func(connection):
  connection.on_connect()
  fill_in_hashes...

keeping in mind that client may have multiple connections and they can be routed to different instances of Redis.

@excitoon
Copy link
Contributor Author

Also, I would recommend to pull backend implementation/interface code from limits, all of these is solved there already.

@excitoon
Copy link
Contributor Author

@Krukov
Copy link
Owner

Krukov commented Mar 17, 2025

Thanks,

I took a look and the main difference is that "limits" uses register_script instead of script_load (https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.core.CoreCommands.register_script)
You can read more here to understand why https://redis-py.readthedocs.io/en/stable/lua_scripting.html :

The Script object ensures that the Lua script is loaded into Redis’s script cache. In the event of a NOSCRIPT error, it will load the script and retry executing it.

So to solve the problem we need to use register_script too. Want to make a PR?

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.

2 participants