Copilot commented on code in PR #2879:
URL: https://github.com/apache/dubbo-go/pull/2879#discussion_r2094554503
##########
registry/nacos/registry_test.go:
##########
@@ -374,3 +376,101 @@ func Test_nacosRegistry_Subscribe(t *testing.T) {
})
}
}
+
+func TestNacosRegistryDestroy(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer ctrl.Finish()
+
+ mockNamingClient := NewMockINamingClient(ctrl)
+ nc := &nacosClient.NacosNamingClient{}
+ nc.SetClient(mockNamingClient)
+
+ regURL, _ := common.NewURL("registry://127.0.0.1:8848")
+ nr := &nacosRegistry{
+ URL: regURL,
+ namingClient: nc,
+ done: make(chan struct{}),
+ registryUrls: []*common.URL{},
+ }
+
+ serviceURL1, _ :=
common.NewURL("dubbo://127.0.0.1:20001/com.example.Service1?interface=com.example.Service1&group=test&version=1.0.0")
+ serviceURL2, _ :=
common.NewURL("dubbo://127.0.0.1:20002/com.example.Service2?interface=com.example.Service2&group=test&version=1.0.0")
+
+ nr.registryUrls = append(nr.registryUrls, serviceURL1)
+ nr.registryUrls = append(nr.registryUrls, serviceURL2)
+
+
mockNamingClient.EXPECT().DeregisterInstance(gomock.Any()).Times(len(nr.registryUrls)).Return(true,
nil)
+
+ nr.Destroy()
+
+ select {
+ case <-nr.done:
+ default:
+ t.Errorf("nr.done channel was not closed after Destroy()")
+ }
+
+ time.Sleep(100 * time.Millisecond)
Review Comment:
Using `time.Sleep` in tests can slow down execution and cause flakiness.
Prefer using channel synchronization or a wait group to deterministically wait
for `nr.done` to close.
```suggestion
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
<-nr.done
}()
wg.Wait()
```
##########
registry/nacos/registry.go:
##########
@@ -48,6 +49,13 @@ import (
const (
LookupInterval = 20 * time.Second
+ checkInterval = 5 * time.Second
+)
+
+var (
Review Comment:
Using global `lastAvailable` and `lastCheckTime` introduces shared state
across all registry instances and may lead to race conditions or unintended
interference. Consider moving these into the `nacosRegistry` struct or using
instance-specific caching.
##########
registry/nacos/registry.go:
##########
@@ -359,20 +370,52 @@ func (nr *nacosRegistry) GetURL() *common.URL {
// IsAvailable determines nacos registry center whether it is available
func (nr *nacosRegistry) IsAvailable() bool {
- // TODO
- return true
+ // Considering both local state + server state
+ select {
+ case <-nr.done:
+ return false
Review Comment:
In handleServiceEvents, if listener.Next() returns an error, the function
returns without closing the listener, which can leak resources. Add
`listener.Close()` before returning in the error branch.
--
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]
