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

Is async version of redis-py coroutine safe? #2835

Open
junyeong-huray opened this issue Jul 6, 2023 · 7 comments
Open

Is async version of redis-py coroutine safe? #2835

junyeong-huray opened this issue Jul 6, 2023 · 7 comments

Comments

@junyeong-huray
Copy link

Hello

I am wondering that async version of redis-py supports coroutine-safety.

I created one redis client instance as a global, and then I used it without any
locking primitives in many coroutines, like in FastAPI request handlers.

from redis.asyncio.cluster import RedisCluster

redis = None


async def initialize_redis():
      global redis
      redis = RedisCluster.from_url(...)
      await redis.initialize()


@app.get('/some/path')
async def handle_get(key: str):
      ...
      v = await redis.get(key)
      ...



@app.put('/some/path')
async def handle_put(key: str, value: str):
      ...
      await redis.set(key, value)
      ...

I didn't experience some kind of bugs with this scenario, but I just want to
make sure that coroutine-safety for redis-py is guranteed.

Thanks,

@chayim
Copy link
Contributor

chayim commented Jul 6, 2023

One can never guarantee coroutine safety. But we definitely aim for it - and shield various parse response calls, due to an issue in the past.

Generally put, you should avoid globals. As I don't develop (yet) with Redis + FastAPI, I don't have an ideal pattern to share unfortunately.

@junyeong-huray
Copy link
Author

@chayim Thank you for answering my question.

Now it's clearer that global async-redis-client can not guarantee
coroutine-safety.

So I am going to make each handlers use exclusive async-redis-client instances.

It seems that there are three options for doing that.

  1. Create an async redis client instance with a newly established connection
    every time.
  2. Create an async redis client instance with a connection pool every time.
  3. Use a pool for async redis client instances.

I guess,

  • option 1. is the best answer if they are compared in terms of bug-free.
  • option 2. is the intended use case of connection pool and it looks decent.
  • option 3. is the most efficient way in terms of cpu cost because it avoid
    instance creation but it is more likely to incur bug since client instances
    are reused.

So I provisionally conclude that option 2. is the best design decision.

If you have any idea please let me know.

Sincerely,

@nsteinmetz
Copy link

nsteinmetz commented Jul 19, 2023

We use case 2 on several projects for a year without troubles (except #2831 with 4.6.0 release)

# app/db/redis.py
async def get_redis_connection():
   redis = Redis.from_url(settings.REDIS_URL)
   return redis

# app/routes/someting.py
@app.put('/some/path')
async def handle_put(key: str, value: str):
     ...
    redis_conn = await get_redis_connection()
    await redis_conn.set(key, value)
    await redis_conn.close()
     ...

We also have some workers where we initialise redis connection in the first function and then pass the redis_conn object to child functions and close it in the first function at the end of the process.

@JerryHuang-LE
Copy link

JerryHuang-LE commented Jul 26, 2023

@nsteinmetz hey mate, just wonder why not do like:

# app/db/redis.py
redis_conn = Redis.from_url(settings.REDIS_URL)

# app/routes/someting.py
@app.put('/some/path')
async def handle_put(key: str, value: str):
     ...
    await redis_conn.set(key, value)
    await redis_conn.close()
     ...

As Redis.from_url(settings.REDIS_URL) will establish the connection_pool I reckon. And it can save some time to establish the connection for each calls.

@nsteinmetz
Copy link

nsteinmetz commented Jul 26, 2023

I would say it's like globals - you can use it but at your own risks ;-)

I'm ok with establishing redis_conn in parent function and pass it to children function till end of the process. But I prefer not to have a global connection.

Matter of taste I would say ; I prefer the least surprise driven development and lose a few ms ;-)

@JerryHuang-LE
Copy link

oh yea you are right. Thanks a lot mate.

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Aug 18, 2023

I am a little confused by the question and the answer here.
The library is an asyncio library, simply making network requests with asyncio primitives. It should be as safe as any other asyncio library.

Obviously, your code can create race conditions in the logic, and you need to keep that in mind (i.e. where you see an await or async in your code, remember that the code may yield to the event loop and run another task), but there should be no issues with the library itself.

The global shouldn't make any difference in this regard, that just seems like a bit of general advice on your programming. For FastAPI, they usually recommend some dependency injection thing in their docs (I think that's a bad idea though, as you won't know that the Redis login is wrong until you've deployed the app and user requests start coming in and failing). In aiohttp, we'd just add it to the app object in a cleanup_ctx:

async def redis_client(app: Application):
    async with redis.Redis() as redis:
        app["redis"] = redis

app.cleanup_ctx.append(redis_client)

Or similar, and then it can easily be accessed in your handlers without any globals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants