feat: Allow for pubsub connections to optionally be taken from pool

[vtermanis]

• Suggestion for Proposal: Optionally allow for pubsub connections to be tracked in pool #2867
• Only applicable to non-cluster client
  • Question: Should it apply to all client types?

[vtermanis]

Only the RE Tests suite fails - it's not able to find an (internal?) docker image. I believe there's nothing I can do about that.

[vtermanis]

@ofekshenawa thank you for taking a look. Are you sure it's a breaking change? The way I've tried to write should mean that both from a behaviour and performance perspective it behaves as before unless the new configuration flag is explicitly set.
(Unless you mean the fact that for now it's only for the non cluster client. I didn't update those so I could age what you, the developers/maintainers, think.)

[github-actions]

This pull request is marked stale. It will be closed in 30 days if it is not updated.

[ndyakov]

@vtermanis thank you for your contribution. What will happen if we exhaust the pool with pub-sub connections, since they are not reusable with resp2?

[Copilot]

PR Overview

This PR introduces the ability to have PubSub connections optionally taken from the connection pool for non-cluster clients. Key changes include:

• Updated tests in pubsub_test.go to verify the behavior when using or not using pool connections.
• Modifications in redis.go and pubsub.go to branch connection handling based on the new PubsubFromPool option.
• Addition of the PubsubFromPool flag in options.go with accompanying documentation.

Reviewed Changes

File	Description
pubsub_test.go	Adds new tests verifying both pool and non-pool connection behavior
redis.go	Implements connection handling for PubSub based on the new option
options.go	Introduces PubsubFromPool flag with explanatory comments
pubsub.go	Adjusts connection function types to support pool-based usage

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pubsub_test.go:592

• [nitpick] A deferred call to pubsub.Close() already exists, making this explicit call potentially redundant. Consider removing one of the closures or ensuring that multiple calls to Close() are idempotent.

pubsub.Close()

[Copilot]

[nitpick] Using context.TODO() here may bypass important context propagation for error handling. Consider refactoring to pass a proper context if available.

Suggested change

	c.pubsubCloseConn = func(conn *pool.Conn) error {
	c.removeConn(context.TODO(), conn, nil)
	c.pubsubCloseConn = func(ctx context.Context, conn *pool.Conn) error {
	c.removeConn(ctx, conn, nil)

[vtermanis]

What will happen if we exhaust the pool with pub-sub connections, since they are not reusable with resp2?

@ndyakov - this should be no different to exhausting the pool with non-pubsub connections, I believe.

1. Normal commands
   1. Connection taken from pool
   2. Connection used for some commands
   3. Connection returned to pool or connection closed (connPool.Remove, if an error occurred)
2. Pub-sub
   1. Connection taken from pool
   2. Connection used for pubsub
   3. Connection closed (connPool.Remove, regardless of success failure)

For both 1.iii & 2.iii, if a connection is closed instead of being returned for re-use, a new connection can be created again, within the total pool limit.
(Caveat: I have not looked at whether the core pool code has changed since my proposal.)