Copilot commented on code in PR #3245:
URL: https://github.com/apache/dubbo-go/pull/3245#discussion_r2934567726
##########
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)
if err != nil {
Review Comment:
`NewClientWithOptions` is passed `context.Background()`, which can never be
canceled. Since other parts of the etcdv3 remoting layer select on
`client.GetCtx().Done()` (e.g., restart handler / listeners), consider creating
a cancelable context tied to the container lifecycle (e.g., cancel when
`container.Done()` is closed) and passing that instead so the client can
reliably terminate/reconnect and avoid goroutine leaks.
##########
remoting/etcdv3/client.go:
##########
@@ -47,7 +51,7 @@ func ValidateClient(container clientFacade, opts
...gxetcd.Option) error {
// Client lose connection with etcd server
if container.Client().GetRawClient() == nil {
- newClient, err := gxetcd.NewClient(options.Name,
options.Endpoints, options.Timeout, options.Heartbeat)
+ newClient, err :=
gxetcd.NewClientWithOptions(context.Background(), options)
if err != nil {
Review Comment:
Same concern here: passing `context.Background()` into
`NewClientWithOptions` makes the client context non-cancelable from this layer.
Please consider deriving a cancelable context (and canceling it on
`container.Done()` / shutdown) so `GetCtx().Done()` can be used to stop
watchers/restart loops and prevent leaks.
##########
remoting/etcdv3/client.go:
##########
@@ -68,7 +72,7 @@ func NewServiceDiscoveryClient(opts ...gxetcd.Option)
*gxetcd.Client {
opt(options)
}
- newClient, err := gxetcd.NewClient(options.Name, options.Endpoints,
options.Timeout, options.Heartbeat)
+ newClient, err := gxetcd.NewClientWithOptions(context.Background(),
options)
if err != nil {
Review Comment:
`NewServiceDiscoveryClient` also hard-codes `context.Background()` for
`NewClientWithOptions`, which prevents callers from propagating
cancellation/deadlines into the etcd client. Consider allowing a
caller-provided context (or otherwise tying cancellation to the service
discovery/registry lifecycle) so internal watches can be terminated cleanly on
shutdown.
--
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]
