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

Add RecyclingMethod to deadpool-redis #386

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

Conversation

ekzhang
Copy link

@ekzhang ekzhang commented Feb 11, 2025

This change adds a new RecyclingMethod enum to deadpool-redis, which can be used to clean up subscriptions when recycling connections.

Also removing the support for configuring Redis connections via AsyncConnectionConfig. Part of the plan in #226

Let me know if there's any other way I could help! (Not sure if you had these changes in another branch also, if that's the case happy to look at that and try to work off of your existing changes.)

This change adds a new `RecyclingMethod` enum to `deadpool-redis`, which can be used to clean up subscriptions when recycling connections.

Also removing the support for configuring Redis connections via `AsyncConnectionConfig`. Part of the plan in deadpool-rs#226

Let me know if there's any other way I could help! (Not sure if you had these changes in another branch also, if that's the case happy to look at that and try to work off of your existing changes.)
@bikeshedder
Copy link
Collaborator

There is one huge downside of this approach: That UNWATCH query is possibly delayed for a quite long time in a pool that's mostly idle. You can essentially end up with a connection with lots of subscriptions that never get used and just waste resources.

The idea I've been meaning to implement is a kind of Subscriber object that one gets from a connection and must actively use. Any (P)SUBSCRIBE query should ideally fail with an error message while the subscriber doesn't exist, yet.

The biggest issue is the lack of a Runtime object though. When an object is dropped it actually needs to be able to spawn a task for cleaning up the object before returning it to the pool.

I guess it's finally the time to look into ahead of time recycling:

I still want to make this opt-in and only perform the ahead of time recycling when passing in a Runtime.

I have to make up my mind how I want this automatic unsubscribe to work. Thanks for the PR! I'll decide if I want to merge this asap.

@ekzhang
Copy link
Author

ekzhang commented Feb 12, 2025

Makes sense! I just ported over the existing code but have not made any actual logical changes yet -- UNWATCH from the existing repository.

Btw I unblocked the build on my project by just making a separate redis::Client object for now but still happy to help with implementing anything if you'd like a hand!

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