Copilot commented on code in PR #3563:
URL: https://github.com/apache/dubbo-go/pull/3563#discussion_r3717992199
##########
registry/protocol/protocol.go:
##########
@@ -86,15 +86,26 @@ func (proto *registryProtocol) getRegistry(registryUrl
*common.URL) registry.Reg
if namespace != "" {
cacheKey = cacheKey + "?" + constant.NacosNamespaceID + "=" +
namespace
}
- actualReg, _ := proto.registries.LoadOrStore(cacheKey, func() any {
- reg, err := extension.GetRegistry(registryUrl.Protocol,
registryUrl)
- if err != nil {
- logger.Errorf("[Registry] registry cannot connect
successfully, err=%s", err.Error())
- panic(err)
- }
- return reg
- }())
- return actualReg.(registry.Registry)
+ if actualReg, loaded := proto.registries.Load(cacheKey); loaded {
+ return cachedRegistry(actualReg, cacheKey)
+ }
+
+ reg, err := extension.GetRegistry(registryUrl.Protocol, registryUrl)
+ if err != nil {
+ logger.Errorf("[Registry] registry cannot connect successfully,
err=%s", err.Error())
+ panic(err)
+ }
Review Comment:
getRegistry still panics on registry connection errors. Since call sites now
handle `reg == nil` (e.g., Refer/Export/Destroy/UnregisterRegistries), consider
returning `nil` (and not storing) instead of panicking to avoid crashing the
whole process on transient/isolated registry failures.
##########
registry/directory/directory.go:
##########
@@ -586,7 +611,7 @@ func (dir *RegistryDirectory)
RemoveClosingInstance(instanceKey string) bool {
defer dir.registerLock.Unlock()
if cacheInvoker, ok := dir.cacheInvokersMap.Load(instanceKey);
ok {
- removed = cacheInvoker.(protocolbase.Invoker)
+ removed, _ = cachedInvoker(instanceKey, cacheInvoker)
}
dir.markClosingTombstone(instanceKey, removed, "closing-event")
removed = dir.uncacheInvokerWithKey(instanceKey)
Review Comment:
In `RemoveClosingInstance`, if `cachedInvoker` returns `ok=false` the
invalid cache entry is left in `cacheInvokersMap`. For consistency with other
call sites (e.g., `toGroupInvokers`, `uncacheInvokerWithClusterID`), consider
deleting `instanceKey` from the cache when an unexpected cached type is
encountered to prevent repeated log noise and lingering corrupted entries.
##########
registry/protocol/protocol.go:
##########
@@ -86,15 +86,26 @@ func (proto *registryProtocol) getRegistry(registryUrl
*common.URL) registry.Reg
if namespace != "" {
cacheKey = cacheKey + "?" + constant.NacosNamespaceID + "=" +
namespace
}
- actualReg, _ := proto.registries.LoadOrStore(cacheKey, func() any {
- reg, err := extension.GetRegistry(registryUrl.Protocol,
registryUrl)
- if err != nil {
- logger.Errorf("[Registry] registry cannot connect
successfully, err=%s", err.Error())
- panic(err)
- }
- return reg
- }())
- return actualReg.(registry.Registry)
+ if actualReg, loaded := proto.registries.Load(cacheKey); loaded {
+ return cachedRegistry(actualReg, cacheKey)
+ }
+
+ reg, err := extension.GetRegistry(registryUrl.Protocol, registryUrl)
+ if err != nil {
+ logger.Errorf("[Registry] registry cannot connect successfully,
err=%s", err.Error())
+ panic(err)
+ }
+ actualReg, _ := proto.registries.LoadOrStore(cacheKey, reg)
+ return cachedRegistry(actualReg, cacheKey)
+}
+
+func cachedRegistry(value any, cacheKey string) registry.Registry {
+ reg, ok := value.(registry.Registry)
+ if !ok || reg == nil {
+ logger.Errorf("[Registry] cached registry has unexpected type
%T for key %s", value, cacheKey)
+ return nil
+ }
+ return reg
Review Comment:
Two related issues: (1) when `cachedRegistry` returns nil due to an
unexpected cached type, the invalid entry remains in `proto.registries`, so all
future callers will keep returning nil (and logging) without any chance to
self-heal; consider deleting the cacheKey (and/or overwriting it with a newly
created registry) when a type mismatch is detected. (2) The
`Load`-then-create-then-`LoadOrStore` pattern can still create duplicate
registry instances under concurrent misses; if registry creation is expensive
or has side effects, consider a singleflight/once-per-key approach to ensure
only one initializer runs per key.
##########
filter/accesslog/filter.go:
##########
@@ -184,6 +178,19 @@ func (f *Filter) buildAccessLogData(_ base.Invoker,
invocation base.Invocation)
return dataMap
}
+func stringAttachment(attachments map[string]any, key string) (string, bool) {
+ value, exists := attachments[key]
+ if !exists || value == nil {
+ return "", false
+ }
+ stringValue, ok := value.(string)
+ if !ok {
+ logger.Warnf("[Filter][AccessLog] attachment %q has unexpected
type %T and will be omitted", key, value)
+ return "", false
+ }
+ return stringValue, true
+}
Review Comment:
Logging a Warn-level message for every non-string attachment can be noisy
and potentially exploitable for log-volume amplification if attachments are
influenced by requests. Consider lowering the level (e.g., Debug) and/or adding
sampling/rate-limiting for this message, while still omitting the invalid
attachment.
##########
protocol/dubbo/dubbo_codec_test.go:
##########
@@ -26,6 +26,12 @@ import (
"github.com/stretchr/testify/require"
)
+import (
+ "dubbo.apache.org/dubbo-go/v3/common/constant"
+ "dubbo.apache.org/dubbo-go/v3/protocol/result"
+ "dubbo.apache.org/dubbo-go/v3/remoting"
+)
Review Comment:
This file now has a second `import (...)` block. In Go, it’s standard to
keep a single import block (letting `gofmt` group stdlib/third-party), and it
also helps avoid accidental duplicate imports of the same path across blocks.
--
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]
