Copilot commented on code in PR #3160:
URL: https://github.com/apache/dubbo-go/pull/3160#discussion_r2689566761
##########
common/url.go:
##########
@@ -869,20 +875,38 @@ func (c *URL) MergeURL(anotherUrl *URL) *URL {
// Clone will copy the URL
func (c *URL) Clone() *URL {
- newURL := &URL{}
- if err := copier.Copy(newURL, c); err != nil {
- // this is impossible
- return newURL
+ newURL := &URL{
+ Protocol: c.Protocol,
+ Location: c.Location,
+ Ip: c.Ip,
+ Port: c.Port,
+ PrimitiveURL: c.PrimitiveURL,
+ Path: c.Path,
+ Username: c.Username,
+ Password: c.Password,
+ Methods: append(make([]string, 0), c.Methods...),
+ attributes: make(map[string]any),
+ params: url.Values{},
+ }
+
+ c.RangeParams(
+ func(key, value string) bool {
+ newURL.SetParam(key, value)
+ return true
+ },
+ )
+
+ c.RangeAttributes(
+ func(key string, value any) bool {
+ newURL.SetAttribute(key, value)
+ return true
+ },
+ )
+
+ if c.SubURL != nil {
+ newURL.SubURL = c.SubURL.Clone()
}
- newURL.params = url.Values{}
- c.RangeParams(func(key, value string) bool {
- newURL.SetParam(key, value)
- return true
- })
- c.RangeAttributes(func(key string, value any) bool {
- newURL.SetAttribute(key, value)
- return true
- })
+
return newURL
}
Review Comment:
The PR description mentions "I have added tests that prove my fix is
effective" to address the concurrent Clone issue from #3159, but no concurrent
test appears to be included in this change. Consider adding a test that spawns
multiple goroutines calling Clone concurrently to verify the thread-safety fix,
similar to the test case shown in issue #3159.
##########
common/url.go:
##########
@@ -897,18 +921,41 @@ func (c *URL) RangeAttributes(f func(key string, value
any) bool) {
}
func (c *URL) CloneExceptParams(excludeParams *gxset.HashSet) *URL {
- newURL := &URL{}
- if err := copier.Copy(newURL, c); err != nil {
- // this is impossible
- return newURL
- }
- newURL.params = url.Values{}
- c.RangeParams(func(key, value string) bool {
- if !excludeParams.Contains(key) {
+ newURL := &URL{
+ Protocol: c.Protocol,
+ Location: c.Location,
+ Ip: c.Ip,
+ Port: c.Port,
+ PrimitiveURL: c.PrimitiveURL,
+ Path: c.Path,
+ Username: c.Username,
+ Password: c.Password,
+ Methods: append(make([]string, 0), c.Methods...),
+ attributes: make(map[string]any),
+ params: url.Values{},
+ }
+
+ c.RangeParams(
+ func(key, value string) bool {
+ if excludeParams != nil && excludeParams.Contains(key) {
+ return true
+ }
newURL.SetParam(key, value)
- }
- return true
- })
+ return true
+ },
+ )
+
+ c.RangeAttributes(
+ func(key string, value any) bool {
+ newURL.SetAttribute(key, value)
+ return true
+ },
+ )
+
+ if c.SubURL != nil {
+ newURL.SubURL = c.SubURL.Clone()
+ }
+
return newURL
}
Review Comment:
Consider adding concurrent tests for CloneExceptParams as well, since it
suffers from the same thread-safety issue that Clone had. The fix properly
addresses the issue by using RangeParams and RangeAttributes with proper
locking, but a test would help prevent regressions.
##########
common/url.go:
##########
@@ -927,24 +974,39 @@ func (c *URL) Compare(comp cm.Comparator) int {
// CloneWithParams Copy URL based on the reserved parameter's keys.
func (c *URL) CloneWithParams(reserveParams []string) *URL {
- params := url.Values{}
+ newURL := &URL{
+ Protocol: c.Protocol,
+ Location: c.Location,
+ Ip: c.Ip,
+ Port: c.Port,
+ PrimitiveURL: c.PrimitiveURL,
+ Path: c.Path,
+ Username: c.Username,
+ Password: c.Password,
+ Methods: append(make([]string, 0), c.Methods...),
+ attributes: make(map[string]any),
+ params: url.Values{},
+ }
+
for _, reserveParam := range reserveParams {
- v := c.GetParam(reserveParam, "")
- if len(v) != 0 {
- params.Set(reserveParam, v)
+ value := c.GetParam(reserveParam, "")
+ if len(value) > 0 {
+ newURL.SetParam(reserveParam, value)
}
}
- return NewURLWithOptions(
- WithProtocol(c.Protocol),
- WithUsername(c.Username),
- WithPassword(c.Password),
- WithIp(c.Ip),
- WithPort(c.Port),
- WithPath(c.Path),
- WithMethods(c.Methods),
- WithParams(params),
+ c.RangeAttributes(
+ func(key string, value any) bool {
+ newURL.SetAttribute(key, value)
+ return true
+ },
)
+
+ if c.SubURL != nil {
+ newURL.SubURL = c.SubURL.Clone()
+ }
+
+ return newURL
}
Review Comment:
Consider adding concurrent tests for CloneWithParams as well, since it also
had the same thread-safety issue. A test would help prevent regressions and
ensure the fix works correctly for all clone methods.
--
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]
