AlexStocks commented on code in PR #3245:
URL: https://github.com/apache/dubbo-go/pull/3245#discussion_r2934570991
##########
remoting/etcdv3/client.go:
##########
@@ -36,7 +40,7 @@ func ValidateClient(container clientFacade, opts
...gxetcd.Option) error {
// new Client
if container.Client() == nil {
- newClient, err := gxetcd.NewClient(options.Name,
options.Endpoints, options.Timeout, options.Heartbeat)
+ newClient, err :=
gxetcd.NewClientWithOptions(context.Background(), options)
Review Comment:
All three `NewClientWithOptions` call sites use `context.Background()`,
which creates an uncancellable context. `ValidateClient` is called under
`container.ClientLock()`, so if etcd is unreachable and the dial blocks, the
lock is held indefinitely — freezing the entire application.
`NewClientWithOptions` internally calls `context.WithCancel(ctx)`, so
cancellation from the caller side is fully supported but unused here.
**Suggestion**: At minimum, use `context.WithTimeout(context.Background(),
options.Timeout)` to bound the blocking time:
```go
ctx, cancel := context.WithTimeout(context.Background(), options.Timeout)
defer cancel()
newClient, err := gxetcd.NewClientWithOptions(ctx, options)
```
Ideally, accept a `context.Context` parameter in `ValidateClient` and
`NewServiceDiscoveryClient` to give callers full control over cancellation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
