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

Error-prone behaviour of SELECT command in combination with connection pool #3124

Open
nj-vs-vh opened this issue Jan 27, 2024 · 1 comment

Comments

@nj-vs-vh
Copy link

nj-vs-vh commented Jan 27, 2024

Version: redis-py 5.0.1, Redis 7.2.4

Platform: Python 3.10.13 on MacOS 13.3.1

Description: Hi! I realize this is not exactly an issue with the library, but I still want to point this out after spending several hours debugging. Consider the following code:

import asyncio
import os
from redis.asyncio import Redis

async def main() -> None:
    r = Redis.from_url(os.environ["REDIS_URL"])
    await r.select(1)
    await r.set("key", b"hello world")

    async def read():
        print(await r.get("key"))

    await asyncio.gather(
        read(),
        read(),
    )

asyncio.run(main())

That code produces (for me, very consistently)

b'hello world'
None

What happens is:

  • the client runs SELECT, which sets db to 1 for one of the connections in the pool (probably the first one)
  • the subsequent SET goes through the same connection and creates "key" in db 1
  • to serve parallel call, an other connection is pulled from the pool, having default db 0 set, and the GET produces no result

Just like that, in a couple lines of code, I have produced an UB and created a bunch of hard-to-find bugs down the line. The root of the problem is that the Redis object abstraction hides the underlying multi-connection complexity. I have read the SELECT documentation page and used the mental model from there to write my Python code, which, it appears, does not apply here.

I suggest possible enhancements:

  • add this caveat to the documentation for select and other stateful connection-level methods (or even create a dedicated section)
  • add helper methods like select_for_all that would run the command for all connections in the pool and set the new default value for the db as new default -- thus making it compatible with single-connection mental model
  • emit a warning or even raise an exception when calling select (and similar stateful connection-level functions), if the client is not in single connection mode; they create an inherent UB since the caller can't know which connection will be modified as a result of the call.
@nj-vs-vh
Copy link
Author

I'll be happy to make a PR, given a green light and some guidance from maintainers.

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

No branches or pull requests

1 participant