Copilot commented on code in PR #3216:
URL: https://github.com/apache/dubbo-go/pull/3216#discussion_r2831746156
##########
cluster/router/chain/chain.go:
##########
@@ -108,6 +112,16 @@ func (c *RouterChain) copyRouters()
[]router.PriorityRouter {
// NewRouterChain init router chain
// Loop routerFactories and call NewRouter method
func NewRouterChain(url *common.URL) (*RouterChain, error) {
+ appNameSetMu.Lock()
+ if url.GetParam(constant.ApplicationKey, "") == "" {
+ if url.SubURL != nil {
+ if appName :=
url.SubURL.GetParam(constant.ApplicationKey, ""); appName != "" {
+ url.SetParam(constant.ApplicationKey, appName)
+ }
+ }
+ }
+ appNameSetMu.Unlock()
Review Comment:
The mutex usage here is unnecessary and incorrect. The URL.SetParam method
already has internal locking (paramsLock) for thread safety, as seen in
common/url.go:558-564. Additionally, this global mutex serializes all
NewRouterChain calls across the entire application, which could become a
performance bottleneck when creating multiple router chains concurrently. Since
URL modification is already thread-safe, the mutex should be removed.
##########
cluster/router/chain/chain.go:
##########
@@ -108,6 +112,16 @@ func (c *RouterChain) copyRouters()
[]router.PriorityRouter {
// NewRouterChain init router chain
// Loop routerFactories and call NewRouter method
func NewRouterChain(url *common.URL) (*RouterChain, error) {
+ appNameSetMu.Lock()
+ if url.GetParam(constant.ApplicationKey, "") == "" {
+ if url.SubURL != nil {
+ if appName :=
url.SubURL.GetParam(constant.ApplicationKey, ""); appName != "" {
+ url.SetParam(constant.ApplicationKey, appName)
+ }
+ }
+ }
+ appNameSetMu.Unlock()
Review Comment:
The PR description claims tests have been added, but there are no new test
files or test functions in this change. A unit test should be added to verify
that the application name is correctly propagated from SubURL when the main URL
doesn't have it set, and that the fix prevents the nil panic in
ApplicationAffinityRouter initialization.
--
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]
