PubSub use of connection pool

[awfm9]

We use go-redis in a production application for our notification protocol between microservices. Initially, we ran into an issue when using more than (I think) 8 pub/sub clients with the same redis client. After some investigating we realized that pub/sub wasn't returning the connections to the connection pool. There are two workarounds: use a single pub/sub for all subscriptions and route the messages to the correct recipients on our own, or increase the maximum number of connections of the pool.

I have just spent some time going through the code of v5 and it does look to me that the pub/sub is now correctly working with the connection pool? Or am I mistaken and we will still need to allocate one connection per pub/sub, permanently?

One a side note, it would be really nice if you could add comments to your code. I, for instance, would certainly have contributed to the project already if it was easier to find my way around the functions. Not even all the public functions are properly commented, which is a real shame. In my opinion, you should even have a lot more than that.

[vmihailenco]

It definitely works - https://github.com/go-redis/redis/blob/v5/pool_test.go#L80-L91. But connection is not returned to the pool until you close subscription with pubsub.Close.

Or am I mistaken and we will still need to allocate one connection per pub/sub, permanently?

Every PubSub allocates one connection. So if you have multiple subscriptions you should keep that in mind.

In my opinion, you should even have a lot more than that.

Agreed. Not that comments will write themselves ;)

[awfm9]

Correct me if I'm wrong, but I don't think it really works that way. From what I can see, closing the pubsub does nothing but closing the underlying base client. This includes closing the connection pool and will make all other pubsubs derived from the same client fail as well, at least as far as I can tell?

Another comment would be that it does not make sense to me why you stopped pubSub() to be exposed publicly. When you initialize an application, you might not know any channels you want to subscribe to yet. Now, you have to create some ugly wrapping code to make sure you initialize the pubsub when you get the first channel.

I don't mean to be rash, but I don't think the pubsub API is well designed in terms of making use of Go mechanics, either. Blocking functions are really ugly in Go. What about returning a channel for each subscription/pattern (or batch of subscriptions/patterns)? I think it would allow more elegant code for pubsubs.

Finally, what is the reason behind creating and using one connection for each pubsub? Personally, I would think the pubsub either needs better support to route the messages of different channels to different consumers, or the pubsub should be an abstraction, with the underlying implementation sharing a connection / connection pool. Right now, it's somehow in-between, which doesn't seem to make too much sense to me.

Looking forward to hear your comments.

[vmihailenco]

at least as far as I can tell?

baseClient has it's own pool of 1 connection used for the PubSub.

Now, you have to create some ugly wrapping code to make sure you initialize the pubsub when you get the first channel.

Probably, but I don't think it is that bad. E.g. you can do

pubsub, err := client.Subscribe("some_dummy_channel")
pubsub.Subscribe("real_channel")

Probably client.Subscribe() should be changed to not require any channels. Overall I don't see a big problem here.

What about returning a channel for each subscription/pattern (or batch of subscriptions/patterns)?

i was thinking about returning one channel for all subscriptions. Anyway both can be done in user space.

needs better support to route the messages of different channels to different consumers, or the pubsub should be an abstraction

You can build this in user space on top of PubSub and share it with everyone. Current PubSub is a minimal set of API that provides you access to Redis channels without any overhead for routing/channels etc. I think that is right foundation.

[awfm9]

Some interesting points, thank you.
It's indeed not a big deal. The dummy channel is not clean, though.
It would be nice to have subscribe just work without channel, I tried that and I think it panicked.
If you do this in user space, you will have trouble cleanly shutting down because of the blocking calls. So you actually have to use the low level function that is recommended against with timeout. By the way, did you consider implementing things like timeouts with contexts yet? That could work really nice with pubsub as well imo.
If you think having a minimal implementation is the way to go (and I actually happen to agree), then I still believe you can't have it use one connection per subscription.

Anyway, thanks a lot for your feedback, it's appreciated.