Skip to content

feat: Option to disable dial on creating client #584

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions rueidis.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ package rueidis
//go:generate go run hack/cmds/gen.go internal/cmds hack/cmds/*.json

import (
"container/list"
"context"
"crypto/tls"
"errors"
"github.com/redis/rueidis/internal/cmds"
"math"
"net"
"runtime"
Expand Down Expand Up @@ -166,6 +168,9 @@ type ClientOption struct {
// ForceSingleClient force the usage of a single client connection, without letting the lib guessing
// if redis instance is a cluster or a single redis instance.
ForceSingleClient bool
// ForceClusterClient forces the usage of a cluster client connection,
// and returns a concrete cluster client despite any of the errors in the dial
ForceClusterClient bool

// ReplicaOnly indicates that this client will only try to connect to readonly replicas of redis setup.
ReplicaOnly bool
Expand Down Expand Up @@ -342,14 +347,47 @@ func NewClient(option ClientOption) (client Client, err error) {
}
if option.Sentinel.MasterSet != "" {
option.PipelineMultiplex = singleClientMultiplex(option.PipelineMultiplex)
return newSentinelClient(&option, makeConn)
sentinelClt, err := newSentinelClient(&option, makeConn)
if err != nil {
// Handle the error gracefully
// TODO: @SoulPancake how to ensure the client can work after Redis is back
return &sentinelClient{
cmd: cmds.NewBuilder(cmds.NoSlot),
mOpt: &option,
sOpt: newSentinelOpt(&option),
connFn: makeConn,
sentinels: list.New(),
retry: !option.DisableRetry,
replica: option.ReplicaOnly,
}, err
}
return sentinelClt, nil
}
if option.ForceSingleClient {
option.PipelineMultiplex = singleClientMultiplex(option.PipelineMultiplex)
return newSingleClient(&option, nil, makeConn)
singleClt, err := newSingleClient(&option, nil, makeConn)
if err != nil {
return &singleClient{
cmd: cmds.NewBuilder(cmds.NoSlot),
conn: nil,
retry: !option.DisableRetry,
DisableCache: option.DisableCache,
}, err
}
return singleClt, nil
}
if client, err = newClusterClient(&option, makeConn); err != nil {
if client == (*clusterClient)(nil) {
if client == (*clusterClient)(nil) && option.ForceClusterClient {
// Return a clusterClient instance if ForceClusterClient is enabled
return &clusterClient{
cmd: cmds.NewBuilder(cmds.InitSlot),
connFn: makeConn,
opt: &option,
conns: make(map[string]connrole),
retry: !option.DisableRetry,
aws: len(option.InitAddress) == 1 && strings.Contains(option.InitAddress[0], "amazonaws.com"),
}, err
} else if client == (*clusterClient)(nil) {
return nil, err
}
if len(option.InitAddress) == 1 && (err.Error() == redisErrMsgCommandNotAllow || strings.Contains(strings.ToUpper(err.Error()), "CLUSTER")) {
Expand Down
100 changes: 73 additions & 27 deletions rueidis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,38 +197,84 @@ func TestFallBackSingleClient(t *testing.T) {

func TestForceSingleClient(t *testing.T) {
defer ShouldNotLeaked(SetupLeakDetection())

ln, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
}
defer ln.Close()
done := make(chan struct{})
go func() {
mock, err := accept(t, ln)
if err != nil {
return
}
mock.Expect("CLIENT", "SETINFO", "LIB-NAME", LibName).
ReplyError("UNKNOWN COMMAND")
mock.Expect("CLIENT", "SETINFO", "LIB-VER", LibVer).
ReplyError("UNKNOWN COMMAND")
mock.Expect("PING").ReplyString("OK")
mock.Close()
close(done)
}()
_, port, _ := net.SplitHostPort(ln.Addr().String())
client, err := NewClient(ClientOption{
InitAddress: []string{"127.0.0.1:" + port},
ForceSingleClient: true,
})
if err != nil {
t.Fatal(err)
}
if _, ok := client.(*singleClient); !ok {
t.Fatal("client should be a singleClient")

testCases := []struct {
name string
maxFailures int
simulateError bool
forceSingleClient bool
expectError bool
clientType string
}{
{"NoFailuresSingleClient", 0, false, true, false, "*rueidis.singleClient"},
{"SimulatedFailuresSingleClient", 3, true, true, true, "*rueidis.singleClient"},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
done := make(chan struct{})

go func() {
mock, err := accept(t, ln)
if err != nil {
return
}
mock.Expect("CLIENT", "SETINFO", "LIB-NAME", LibName).
ReplyError("UNKNOWN COMMAND")
mock.Expect("CLIENT", "SETINFO", "LIB-VER", LibVer).
ReplyError("UNKNOWN COMMAND")
mock.Expect("PING").ReplyString("OK")
mock.Close()
close(done)
}()

_, port, _ := net.SplitHostPort(ln.Addr().String())

dialFn := func(addr string, dialer *net.Dialer, tlsConfig *tls.Config) (net.Conn, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rueian Do you think this is how I should do it?
In principle it should work but I am not too sure why it is timing out, maybe I'm missing something

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dailFn looks good to me but you must invoke client.Do to trigger the dailFn again after the NewClient. Otherwise, the dailFn is only called once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly stuck here
I should client.Do to trigger the dialFn right after the NewClient call? @rueian

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I believe so. There is no other background goroutine that will dial for a connection.

if tc.simulateError && tc.maxFailures > 0 {
tc.maxFailures--
return nil, fmt.Errorf("simulated connection failure")
}
if tlsConfig != nil {
return tls.DialWithDialer(dialer, "tcp", addr, tlsConfig)
}
return dialer.Dial("tcp", addr)
}

client, err := NewClient(ClientOption{
InitAddress: []string{"127.0.0.1:" + port},
ForceSingleClient: tc.forceSingleClient,
DialFn: dialFn,
})

// Check for expected error based on test case
if tc.expectError {
if err == nil {
t.Fatalf("expected an error but got nil")
}
} else {
if err != nil {
t.Fatalf("did not expect an error but got: %v", err)
}
}

// Check the type of the returned client
clientType := fmt.Sprintf("%T", client)
if clientType != tc.clientType {
t.Fatalf("expected client to be of type %s, but got %T", tc.clientType, client)
}
if !tc.expectError {
client.Close()
}
<-done
})
}
client.Close()
<-done
}

func TestTLSClient(t *testing.T) {
Expand Down Expand Up @@ -321,7 +367,7 @@ func TestNewClientMaxMultiplex(t *testing.T) {
InitAddress: []string{"127.0.0.1:6379"},
PipelineMultiplex: MaxPipelineMultiplex + 1,
})
if err != ErrWrongPipelineMultiplex {
if !errors.Is(err, ErrWrongPipelineMultiplex) {
t.Fatalf("unexpected error %v", err)
}
}
Expand Down
Loading