From 055b6d7b19944fed615f317f3b901e7698c941a5 Mon Sep 17 00:00:00 2001 From: Yi-Shu Tai Date: Thu, 8 Jul 2021 16:41:31 -0700 Subject: [PATCH 1/2] client: call .Endpoints() in dial() in client/v3/client.go instead of accessing cfg.Endpoints directly https://github.com/etcd-io/etcd/blob/0cdd558361c6bdbbd9e4023558e2f6ece71c18ad/client/v3/client.go#L299 accesses endpoints without acquiring lock. Fix it to call Endpoints() Fix #13201 Signed-off-by: Chao Chen --- clientv3/client.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/clientv3/client.go b/clientv3/client.go index 3fc75c15f36..e16d04be524 100644 --- a/clientv3/client.go +++ b/clientv3/client.go @@ -56,7 +56,9 @@ type Client struct { cfg Config creds grpccredentials.TransportCredentials resolver *resolver.EtcdManualResolver - mu *sync.RWMutex + + epMu *sync.RWMutex + endpoints []string ctx context.Context cancel context.CancelFunc @@ -140,18 +142,18 @@ func (c *Client) Ctx() context.Context { return c.ctx } // Endpoints lists the registered endpoints for the client. func (c *Client) Endpoints() []string { // copy the slice; protect original endpoints from being changed - c.mu.RLock() - defer c.mu.RUnlock() - eps := make([]string, len(c.cfg.Endpoints)) - copy(eps, c.cfg.Endpoints) + c.epMu.RLock() + defer c.epMu.RUnlock() + eps := make([]string, len(c.endpoints)) + copy(eps, c.endpoints) return eps } // SetEndpoints updates client's endpoints. func (c *Client) SetEndpoints(eps ...string) { - c.mu.Lock() - defer c.mu.Unlock() - c.cfg.Endpoints = eps + c.epMu.Lock() + defer c.epMu.Unlock() + c.endpoints = eps c.resolver.SetEndpoints(eps) } @@ -279,7 +281,7 @@ func (c *Client) dial(creds grpccredentials.TransportCredentials, dopts ...grpc. defer cancel() // TODO: Is this right for cases where grpc.WithBlock() is not set on the dial options? } - initialEndpoints := strings.Join(c.cfg.Endpoints, ";") + initialEndpoints := strings.Join(c.Endpoints(), ";") target := fmt.Sprintf("%s://%p/#initially=[%s]", resolver.Schema, c, initialEndpoints) conn, err := grpc.DialContext(dctx, target, opts...) if err != nil { @@ -327,7 +329,7 @@ func newClient(cfg *Config) (*Client, error) { creds: creds, ctx: ctx, cancel: cancel, - mu: new(sync.RWMutex), + epMu: new(sync.RWMutex), callOpts: defaultCallOpts, lgMu: new(sync.RWMutex), } @@ -369,6 +371,7 @@ func newClient(cfg *Config) (*Client, error) { if len(cfg.Endpoints) < 1 { return nil, fmt.Errorf("at least one Endpoint must is required in client config") } + client.SetEndpoints(cfg.Endpoints...) // Use a provided endpoint target so that for https:// without any tls config given, then // grpc will assume the certificate server name is the endpoint host. From 97a6f957b24483d7561b4e576da15ec77a654ef9 Mon Sep 17 00:00:00 2001 From: Chao Chen Date: Tue, 31 Oct 2023 09:43:06 -0700 Subject: [PATCH 2/2] Fix unit test unreferenced mu in clientv3/client_test.go Signed-off-by: Chao Chen --- clientv3/client_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clientv3/client_test.go b/clientv3/client_test.go index dccc55b9f78..b669cfe5e81 100644 --- a/clientv3/client_test.go +++ b/clientv3/client_test.go @@ -237,14 +237,12 @@ func TestClientRejectOldCluster(t *testing.T) { endpointToVersion[tt.endpoints[j]] = tt.versions[j] } c := &Client{ - ctx: context.Background(), - cfg: Config{ - Endpoints: tt.endpoints, - }, + ctx: context.Background(), + endpoints: tt.endpoints, + epMu: new(sync.RWMutex), Maintenance: &mockMaintenance{ Version: endpointToVersion, }, - mu: new(sync.RWMutex), } if err := c.checkVersion(); err != tt.expectedError {