Skip to content

Conversation

@yishuT
Copy link
Contributor

@yishuT yishuT commented Jul 9, 2021

initialEndpoints := strings.Join(c.cfg.Endpoints, ";")
accesses
endpoints without acquiring lock. Fix it to call Endpoints()

Fix #13201

@serathius
Copy link
Member

I agree that this variable needs a lock to prevent data race with SetEndpoints, but I would also not expect that SetEndpoints should modify Config.
I would not expect config structs like client.v3.Config to be ever mutated by consumers. I expect that SetEndpoints was added later. Mutex withing Client doesn't directly specify which variables are protected by it.

Instead of of just adding lock, maybe we should consider adding dedicated endpoint field to Client struct and specifying explicitly that is protected by mutex?

@yishuT
Copy link
Contributor Author

yishuT commented Jul 9, 2021

yeah, I agree. I only fixed what our race detector complained but didn't read the code in detail. There is one thing that if SetEndpoints is called for a new list of endpoints after client is init, now this client has two different list of endpoints, one in client.cfg, the other in client.endpoint.

@yishuT
Copy link
Contributor Author

yishuT commented Jul 9, 2021

nvm, we don't do deep copy in SetEndpoints

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

LG

@yishuT
Copy link
Contributor Author

yishuT commented Jul 18, 2021

any updates, or should I do something?

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

lgtm. Can you please squash commits into one? Thanks @yishuT

@yishuT
Copy link
Contributor Author

yishuT commented Jul 27, 2021

squash commits

@spzala
Copy link
Member

spzala commented Jul 29, 2021

squash commits

Thanks!
CI is good, also tested locally.

@spzala spzala merged commit 4cbb949 into etcd-io:main Jul 29, 2021
@yishuT yishuT deleted the client-race branch July 29, 2021 09:06
chaochn47 added a commit to chaochn47/etcd that referenced this pull request Oct 26, 2023
…lient/v3/client.go instead of accessing cfg.Endpoints directly

Signed-off-by: Chao Chen <[email protected]>
chaochn47 added a commit to chaochn47/etcd that referenced this pull request Oct 27, 2023
…lient/v3/client.go instead of accessing cfg.Endpoints directly

Signed-off-by: Chao Chen <[email protected]>
@twz123
Copy link
Contributor

twz123 commented Oct 15, 2025

Now that this PR is available in etcd 3.6, there's some GRPC warnings being issued when creating a client via client.New:

2025/10/15 08:16:40 WARNING: [core] [Channel #1 SubChannel #2] grpc: addrConn.createTransport failed to connect to {Addr: "127.0.0.1:2379", ServerName: "127.0.0.1", }. Err: connection error: desc = "transport: Error while dialing: dial tcp 127.0.0.1:2379: operation was canceled"

Is that expected? Everything else works as intended, New returns without error, and the returned client is usable. However, a "warning" implies that something is not as it should be... 🤔

client.cancel()
return nil, fmt.Errorf("at least one Endpoint is required in client config")
}
client.SetEndpoints(cfg.Endpoints...)
Copy link
Contributor

Choose a reason for hiding this comment

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

This calls client.resolver.SetEndpoints(eps), which hasn't been called before. I cannot really judge if this is a problem or not, but I observe GRPC warnings being logged with this:

2025/10/15 12:55:29 WARNING: [core] [Channel #1 SubChannel #2] grpc: addrConn.createTransport failed to connect to {Addr: "127.0.0.1:2379", ServerName: "127.0.0.1:2379", }. Err: connection error: desc = "transport: Error while dialing: dial tcp 127.0.0.1:2379: operation was canceled"

or

2025/10/15 12:56:24 WARNING: [core] [Channel #1 SubChannel #2] grpc: addrConn.createTransport failed to connect to {Addr: "127.0.0.1:2379", ServerName: "127.0.0.1:2379", }. Err: connection error: desc = "transport: authentication handshake failed: context canceled"

Replacing this with the following makes them disappear again:

Suggested change
client.SetEndpoints(cfg.Endpoints...)
client.epMu.Lock()
client.endpoints = cfg.Endpoints
client.epMu.Unlock()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

accessing cfg.Endpoints in dial() in client/v3/client.go has race with SetEndpoints

4 participants