Better connection design for pooling

[musjj]

I was trying to use deadpool with redis-rs, but got stuck when trying to use pubsub with it: bikeshedder/deadpool#226 (comment).

It's also a problem for bb8: djc/bb8#70

It looks like that the way the connection objects are designed makes it hard for third-parties to integrate with it. Quoting the deadpool maintainer @bikeshedder:

I just checked the documentation and example code from the current redis version and it seams that PubSub now works very different from previous versions. Instead of calling into_pubsub you create a dedicated PubSub object via the Client::get_async_pubsub method.

After a bit more digging it is possible to convert a Connection into a PubSub but there doesn't seam to be a way to get back the original connection once it's been converted into a PubSub object.

I currently see only one way for this to work:

* Create a separate pool implementation just for `PubSub` connections. e.g. `deadpool_redis::PubSubPool` which creates `PubSub` connections.

It really is a bummer that the redis crate makes a distinction between Connection, PubSub and Monitor which are all different types of connections which can't be interchanged.

It's weird as the MultiplexedConnection even provides methods for (un)subscribing but there doesn't seam to be a way to access the published messages: https://docs.rs/redis/latest/redis/aio/struct.MultiplexedConnection.html

I'm not really sure what's the best way forward. Maybe we can have a unified connection object, instead of splitting them? What do you think?

[bikeshedder]

Thank you for opening a redis-rs/redis-rs issue for that. 👍

I just discovered get_multiplexed_async_connection_with_config and AsyncConnectionConfig which does provide a set_push_sender method.

It does have a completely different API though. It doesn't use the Msg struct but PushInfo which still requires a bit of parsing to be actually usable.

I guess it could be made to work in deadpool-redis but I'd really prefer this to be a feature of the redis crate and not deadpool-redis.

[nihohit]

It really is a bummer that the redis crate makes a distinction between Connection, PubSub and Monitor which are all different types of connections which can't be interchanged.

The problem is that the Redis RESP2 makes this distinction. Once a connection is made into a pubsub connection, sending messages on it breaks the pubsub. The way to overcome it is to use RESP3 pubsub, as @bikeshedder mentioned. I don't see a way to overcome this distinction between types for RESP2 - suggestions are welcome.

but PushInfo which still requires a bit of parsing to be actually usable.

What's the issue you're experiencing? What interface would you like to see? Something like #1437?

[nihohit]

It's weird as the MultiplexedConnection even provides methods for (un)subscribing but there doesn't seam to be a way to access the published messages:

I see that our current documentation isn't clear enough about this. Would this help?
#1436

[bikeshedder]

What's the issue you're experiencing? What interface would you like to see? Something like #1437?

• Msg does provide methods like get_channel, get_channel_name, get_payload, etc.
• PushInfo just provides a Vec<Value>

It would be nice if PushInfo would provide that data in a ready parsed form so it can be consumed more easily.

I guess the first entry in PushInfo::data is the topic/channel name? It would be nice if this data was consumable in a simpler form similar to what Msg does.

Edit: Yes, exactly something like #1437 😅
Edit2: Oh my, I didn't notice the Msg::from_push_info method. 🤦

[bikeshedder]

I'm currently looking into ways to implement this feature in deadpool-redis. As of now there is no way to disable that channel while it is not being used. It would be nice if there was a way to control the way the push_sender behaves. e.g. a Box<dyn Fn(PushInfo)> would allow for providing a callback that could drop messages if there is no receiver waiting for them.

Alternatively a tokio::sync::broadcast channel could be a more sensible choice as it drops messages if there are no receivers currently subscribed.

With the current API I would always need to spawn a tokio task that reads from the channel even if the connection is currently not used for subscriptions.

[nihohit]

Alternatively a tokio::sync::broadcast channel could be a more sensible choice as it drops messages if there are no receivers currently subscribed.

So, like this?
#1295

[bikeshedder]

Yes, #1295 does indeed solve this. Any chance getting this merged for the next release of the redis crate? Adding PubSub support in deadpool-redis is a long standing issue and has been asked for a lot.

[nihohit]

released 0.27.6 with the updated docs and more lenient push senders - I took your suggestion, and also enabled passing closures, not only senders.

[bikeshedder]

Very nice. I need to get working on deadpool-redis then and add PubSub support asap. 🥳

[nihohit]

@bikeshedder @musjj anything else that is needed, or can this issue be closed?

[bikeshedder]

I think both the documentation and the API are much better now. 👍

I don't see anything blocking PubSub support in pools anymore. 🥳

[musjj]

Yeah, I don't think there's any blockers anymore!