Copilot commented on code in PR #3054:
URL: https://github.com/apache/dubbo-go/pull/3054#discussion_r2441515274
##########
registry/protocol/protocol_test.go:
##########
@@ -72,9 +77,7 @@ func referNormal(t *testing.T, regProtocol *registryProtocol)
{
}
func TestRefer(t *testing.T) {
- config.SetRootConfig(config.RootConfig{
- Application: &config.ApplicationConfig{Name:
"test-application"},
- })
+
Review Comment:
[nitpick] Empty line 80 creates unnecessary whitespace. Remove the blank
line to follow the project's formatting conventions for test functions.
```suggestion
```
##########
registry/protocol/protocol.go:
##########
@@ -185,12 +186,33 @@ func (proto *registryProtocol) Refer(url *common.URL)
base.Invoker {
// Export provider service to registry center
func (proto *registryProtocol) Export(originInvoker base.Invoker)
base.Exporter {
- proto.once.Do(func() {
- proto.initConfigurationListeners()
- })
registryUrl := getRegistryUrl(originInvoker)
providerUrl := getProviderUrl(originInvoker)
+ if _, ok := registryUrl.GetAttribute(constant.ShutdownConfigPrefix);
!ok {
+ // Only set default global config when config package doesn't
have one
+ if config.GetShutDown() == nil {
Review Comment:
The condition should check if the attribute is not present AND config
package has a value. Current logic sets default global config when config
package is nil, which contradicts the stated purpose of 'Only set default
global config when config package doesn't have one'. The logic should be
inverted to check if config.GetShutDown() != nil and use it, otherwise set the
default.
```suggestion
if config.GetShutDown() != nil {
registryUrl.SetAttribute(constant.ShutdownConfigPrefix,
config.GetShutDown())
} else {
```
##########
registry/protocol/protocol.go:
##########
@@ -185,12 +186,33 @@ func (proto *registryProtocol) Refer(url *common.URL)
base.Invoker {
// Export provider service to registry center
func (proto *registryProtocol) Export(originInvoker base.Invoker)
base.Exporter {
- proto.once.Do(func() {
- proto.initConfigurationListeners()
- })
registryUrl := getRegistryUrl(originInvoker)
providerUrl := getProviderUrl(originInvoker)
+ if _, ok := registryUrl.GetAttribute(constant.ShutdownConfigPrefix);
!ok {
+ // Only set default global config when config package doesn't
have one
+ if config.GetShutDown() == nil {
+ registryUrl.SetAttribute(constant.ShutdownConfigPrefix,
global.DefaultShutdownConfig())
+ }
+ }
+
+ // Copy ApplicationKey from registryUrl to providerUrl if providerUrl
doesn't have it
+ if _, ok := providerUrl.GetAttribute(constant.ApplicationKey); !ok {
+ if appConfigRaw, ok :=
registryUrl.GetAttribute(constant.ApplicationKey); ok {
+ // Use ApplicationConfig from registryUrl (new API)
+ providerUrl.SetAttribute(constant.ApplicationKey,
appConfigRaw)
+ } else {
+ // Only set default global config when config package
doesn't have one
+ if config.GetRootConfig() == nil ||
config.GetRootConfig().Application == nil {
Review Comment:
config.GetRootConfig() is called twice when the first call returns non-nil.
Store the result in a variable to avoid redundant function calls.
```suggestion
rootConfig := config.GetRootConfig()
if rootConfig == nil || rootConfig.Application == nil {
```
##########
registry/protocol/protocol.go:
##########
@@ -424,9 +474,24 @@ func (proto *registryProtocol) Destroy() {
// close all protocol server after consumerUpdateWait +
stepTimeout(max time wait during
// waitAndAcceptNewRequests procedure)
go func() {
- <-time.After(config.GetShutDown().GetStepTimeout() +
config.GetShutDown().GetConsumerUpdateWaitTime())
- exporter.UnExport()
- proto.bounds.Delete(key)
+ // 1. Prefer global config from URL attribute
(including default set in Export)
+ if shutdownConfRaw, ok :=
exporter.registerUrl.GetAttribute(constant.ShutdownConfigPrefix); ok {
+ if shutdownConfig, ok :=
shutdownConfRaw.(*global.ShutdownConfig); ok {
+ stepTimeout, _ :=
time.ParseDuration(shutdownConfig.StepTimeout)
+ consumerUpdateWaitTime, _ :=
time.ParseDuration(shutdownConfig.ConsumerUpdateWaitTime)
+ <-time.After(stepTimeout +
consumerUpdateWaitTime)
+ exporter.UnExport()
+ proto.bounds.Delete(key)
+ return
+ }
+ }
+
+ // 2. Fallback to config package (already set in Export
if exists)
+ if configShutdown := config.GetShutDown();
configShutdown != nil {
+ <-time.After(configShutdown.GetStepTimeout() +
configShutdown.GetConsumerUpdateWaitTime())
+ exporter.UnExport()
+ proto.bounds.Delete(key)
+ }
Review Comment:
When configShutdown is nil, the goroutine exits without calling UnExport()
or cleaning up proto.bounds. This creates a resource leak. Either log a warning
and perform cleanup, or move the cleanup outside the conditional.
```suggestion
proto.bounds.Delete(key)
return
}
// 3. No shutdown config found, log warning and cleanup
immediately
logger.Warnf("[registryProtocol] No shutdown config
found for exporter %v, cleaning up immediately to avoid resource leak.", key)
exporter.UnExport()
proto.bounds.Delete(key)
```
--
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]
