NewClient with lazy connections

[justinmayhew]

Hi Rueian, thanks for creating this great package and sharing it with all of us.

I'm new to Rueidis and one thing that surprised me is that rueidis.NewClient eagerly connects to Redis. For example:

start := time.Now()
client, err := rueidis.NewClient(rueidis.ClientOption{
    InitAddress:       []string{"127.0.0.1:6379"},
    ForceSingleClient: true,
    DisableCache:      true,
    PipelineMultiplex: -1,
})
if err != nil {
    fmt.Println("Unable to connect to Redis:", err)
} else {
    fmt.Printf("Connected to Redis in %s\n", time.Since(start))
}
defer client.Close()

Even if there's an err, the client can still be used and will gracefully attempt to connect on future calls. This resilience is great.

What are your thoughts on adding the ability to lazily connect on the first call, similar to pgxpool?

A pool returns without waiting for any connections to be established. Acquire a connection immediately after creating the pool to check if a connection can successfully be established.

I think this is preferable in many situations because it allows us to defer connecting until it's actually needed. This can speed up HTTP server initialization, for example.

[rueian]

Even if there's an err, the client can still be used and will gracefully attempt to connect on future calls. This resilience is great.

Yes, but keep in mind that this behavior is only available when ForceSingleClient is set. For other cases, the client will be nil if there is an err returned.

What are your thoughts on adding the ability to lazily connect on the first call, similar to pgxpool?

I think the lazy connect can be achieved easily with a new wrapper which implements the rueidis.Client interface again by proxying all the function calls to an actual rueidis.Client stored in an atomic.Value in the wrapper. The wrapper will only initiate a rueidis.NewClient when the atomic.Value is nil and store the created client to the atomic.Value if there is no err returned.

Would you like to contribute to the wrapper?

[justinmayhew]

This change works well in my testing. It's a behaviour change that would probably be unexpected by some users, but it accomplishes the goal of connecting lazily. We could add a new field to ClientOption to opt-in to this. What do you think of the general approach?

diff --git a/client.go b/client.go
index ed907e8..50bf1ad 100644
--- a/client.go
+++ b/client.go
@@ -29,15 +29,14 @@ func newSingleClient(opt *ClientOption, prev conn, connFn connFn, retryer retryH
        }

        conn := connFn(opt.InitAddress[0], opt)
-       conn.Override(prev)
-       err := conn.Dial()
-       if err != nil {
-               if !opt.ForceSingleClient {
+       if !opt.ForceSingleClient { // ForceSingleClient doesn't connect until the first command.
+               conn.Override(prev)
+               err := conn.Dial()
+               if err != nil {
                        return nil, err
                }
        }
-       // return a singleClient instance even when Dial fails if ForceSingleClient is true.
-       return newSingleClientWithConn(conn, cmds.NewBuilder(cmds.NoSlot), !opt.DisableRetry, opt.DisableCache, retryer, opt.ConnLifetime > 0), err
+       return newSingleClientWithConn(conn, cmds.NewBuilder(cmds.NoSlot), !opt.DisableRetry, opt.DisableCache, retryer, opt.ConnLifetime > 0), nil
 }

 func newSingleClientWithConn(conn conn, builder Builder, retry, disableCache bool, retryer retryHandler, hasLftm bool) *singleClient {

[rueian]

Isn't the existing behavior of singleClient sufficient if we avoid this call to Dial?
Could we add another option that avoids running that Dial, or even do it unconditionally for ForceSingleClient (although I guess that would be a breaking change)?

Yes, and yes, that will be a breaking change, unfortunately. However, I think it is okay to have a new environment variable to avoid the Dial call when ForceSingleClient is set.

[justinmayhew]

I'd prefer to add a field to ClientOption because I don't think it's a good idea for libraries to have magic environment variables. What do you think?

Also if we add this functionality, do you think it makes sense for other Redis configurations like Cluster and Sentinel, or does it only make sense for the single client use case? I want to make sure if we add this it's well thought out and works for everyone, not special cased for my specific use case.

[rueian]

Okay, just make sure the name of the new option is general and short (SkipInitDial?), and is not limited to a single client. However, we do need to validate if ForceSingleClient is set simultaneously for now.

[justinmayhew]

Sounds good, thank you. I'll try to implement this.