This is an automated email from the ASF dual-hosted git repository.
alexstocks pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/dubbo-go.git
The following commit(s) were added to refs/heads/develop by this push:
new f1ac66655 fix(common): Make URL cloning thread-safe (#3160)
f1ac66655 is described below
commit f1ac666550e9e1e951ed980c11a7a88bb7fd51e8
Author: 陈钢阳 <[email protected]>
AuthorDate: Tue Jan 27 16:19:58 2026 +0800
fix(common): Make URL cloning thread-safe (#3160)
This commit fixes a critical thread-safety issue in the Clone and
CloneExceptParams methods of the URL struct.
The previous implementation used the github.com/jinzhu/copier library,
which is not safe for types containing mutexes. This caused two main problems
in a concurrent environment:
1. Data Races: The library read fields from the source URL without
acquiring a lock, leading to data races.
2. Mutex Copying: The sync.RWMutex fields within the URL were being copied,
which is an incorrect usage of mutexes and leads to unpredictable behavior.
The implementation has been refactored to manually construct a new URL
instance and explicitly copy each field. This new approach ensures that a
cloned URL has its own new, properly initialized mutexes, making the cloning
operation safe for concurrent use. This also has the added benefit of removing
the copier dependency.
(cherry picked from commit 589c998c2bb19d3999316ac529c52ef1c4c5216f)
fix(common): Make URL cloning thread-safe
This commit fixes a critical thread-safety issue in the Clone and
CloneExceptParams methods of the URL struct.
The previous implementation used the github.com/jinzhu/copier library,
which is not safe for types containing mutexes. This caused two main problems
in a concurrent environment:
1. Data Races: The library read fields from the source URL without
acquiring a lock, leading to data races.
2. Mutex Copying: The sync.RWMutex fields within the URL were being copied,
which is an incorrect usage of mutexes and leads to unpredictable behavior.
The implementation has been refactored to manually construct a new URL
instance and explicitly copy each field. This new approach ensures that a
cloned URL has its own new, properly initialized mutexes, making the cloning
operation safe for concurrent use. This also has the added benefit of removing
the copier dependency.
fix(common): Make URL cloning thread-safe
This commit fixes a critical thread-safety issue in the Clone and
CloneExceptParams methods of the URL struct.
The previous implementation used the github.com/jinzhu/copier library,
which is not safe for types containing mutexes. This caused two main problems
in a concurrent environment:
1. Data Races: The library read fields from the source URL without
acquiring a lock, leading to data races.
2. Mutex Copying: The sync.RWMutex fields within the URL were being copied,
which is an incorrect usage of mutexes and leads to unpredictable behavior.
The implementation has been refactored to manually construct a new URL
instance and explicitly copy each field. This new approach ensures that a
cloned URL has its own new, properly initialized mutexes, making the cloning
operation safe for concurrent use. This also has the added benefit of removing
the copier dependency.
fix(common): Make URL cloning thread-safe
This commit fixes a critical thread-safety issue in the Clone and
CloneExceptParams methods of the URL struct.
The previous implementation used the github.com/jinzhu/copier library,
which is not safe for types containing mutexes. This caused two main problems
in a concurrent environment:
1. Data Races: The library read fields from the source URL without
acquiring a lock, leading to data races.
2. Mutex Copying: The sync.RWMutex fields within the URL were being copied,
which is an incorrect usage of mutexes and leads to unpredictable behavior.
The implementation has been refactored to manually construct a new URL
instance and explicitly copy each field. This new approach ensures that a
cloned URL has its own new, properly initialized mutexes, making the cloning
operation safe for concurrent use. This also has the added benefit of removing
the copier dependency.
---
common/url.go | 134 +++++++++++++++++++++++++++++------------------------
common/url_test.go | 47 +++++++++++++++++++
go.mod | 1 -
go.sum | 2 -
4 files changed, 121 insertions(+), 63 deletions(-)
diff --git a/common/url.go b/common/url.go
index f90031b98..8f5cd7895 100644
--- a/common/url.go
+++ b/common/url.go
@@ -24,6 +24,7 @@ import (
"math"
"net"
"net/url"
+ "slices"
"strconv"
"strings"
"sync"
@@ -37,8 +38,6 @@ import (
"github.com/google/uuid"
- "github.com/jinzhu/copier"
-
perrors "github.com/pkg/errors"
)
@@ -408,8 +407,10 @@ func (c *URL) String() string {
// Key gets key
func (c *URL) Key() string {
- buildString :=
fmt.Sprintf("%s://%s:%s@%s:%s/?interface=%s&group=%s&version=%s",
- c.Protocol, c.Username, c.Password, c.Ip, c.Port, c.Service(),
c.GetParam(constant.GroupKey, ""), c.GetParam(constant.VersionKey, ""))
+ buildString := fmt.Sprintf(
+ "%s://%s:%s@%s:%s/?interface=%s&group=%s&version=%s",
+ c.Protocol, c.Username, c.Password, c.Ip, c.Port, c.Service(),
c.GetParam(constant.GroupKey, ""), c.GetParam(constant.VersionKey, ""),
+ )
return buildString
}
@@ -417,17 +418,21 @@ func (c *URL) Key() string {
func (c *URL) GetCacheInvokerMapKey() string {
urlNew, _ := NewURL(c.PrimitiveURL)
- buildString :=
fmt.Sprintf("%s://%s:%s@%s:%s/?interface=%s&group=%s&version=%s×tamp=%s&"+constant.MeshClusterIDKey+"=%s",
+ buildString := fmt.Sprintf(
+
"%s://%s:%s@%s:%s/?interface=%s&group=%s&version=%s×tamp=%s&"+constant.MeshClusterIDKey+"=%s",
c.Protocol, c.Username, c.Password, c.Ip, c.Port, c.Service(),
c.GetParam(constant.GroupKey, ""),
c.GetParam(constant.VersionKey, ""),
urlNew.GetParam(constant.TimestampKey, ""),
- c.GetParam(constant.MeshClusterIDKey, ""))
+ c.GetParam(constant.MeshClusterIDKey, ""),
+ )
return buildString
}
// ServiceKey gets a unique key of a service.
func (c *URL) ServiceKey() string {
- return ServiceKey(c.GetParam(constant.InterfaceKey,
strings.TrimPrefix(c.Path, constant.PathSeparator)),
- c.GetParam(constant.GroupKey, ""),
c.GetParam(constant.VersionKey, ""))
+ return ServiceKey(
+ c.GetParam(constant.InterfaceKey, strings.TrimPrefix(c.Path,
constant.PathSeparator)),
+ c.GetParam(constant.GroupKey, ""),
c.GetParam(constant.VersionKey, ""),
+ )
}
func ServiceKey(intf string, group string, version string) string {
@@ -767,10 +772,12 @@ func (c *URL) SetParams(m url.Values) {
func (c *URL) ToMap() map[string]string {
paramsMap := make(map[string]string)
- c.RangeParams(func(key, value string) bool {
- paramsMap[key] = value
- return true
- })
+ c.RangeParams(
+ func(key, value string) bool {
+ paramsMap[key] = value
+ return true
+ },
+ )
if c.Protocol != "" {
paramsMap[PROTOCOL] = c.Protocol
@@ -845,11 +852,11 @@ func (c *URL) MergeURL(anotherUrl *URL) *URL {
}
methodsKey := "methods." + method + "." + paramKey
- //if len(mergedURL.GetParam(methodsKey, "")) == 0 {
+ // if len(mergedURL.GetParam(methodsKey, "")) == 0 {
if v := anotherUrl.GetParam(methodsKey, ""); len(v) > 0
{
params[methodsKey] = []string{v}
}
- //}
+ // }
mergedURL.Methods[i] = method
}
}
@@ -867,25 +874,61 @@ func (c *URL) MergeURL(anotherUrl *URL) *URL {
return mergedURL
}
-// 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
+// CloneWithFilter - Clone the URL with parameter filtering
+// excludeParams: the set of parameters to exclude from the cloned URL
+// reserveParams: the set of parameters to retain in the cloned URL
+func (c *URL) CloneWithFilter(excludeParams *gxset.HashSet, reserveParams
[]string) *URL {
+ 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{},
+ }
+
+ // Copy and filter params based on excludeParams or reserveParams
+ c.RangeParams(
+ func(key, value string) bool {
+ // If the param is in excludeParams or not in
reserveParams, skip it
+ if excludeParams != nil && excludeParams.Contains(key) {
+ return true
+ }
+ if len(reserveParams) > 0 &&
!slices.Contains(reserveParams, key) {
+ return true
+ }
+ // Set the param if it passes the filter
+ newURL.SetParam(key, value)
+ return true
+ },
+ )
+
+ // Copy attributes
+ c.RangeAttributes(
+ func(key string, value any) bool {
+ newURL.SetAttribute(key, value)
+ return true
+ },
+ )
+
+ // Copy SubURL if it exists
+ 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
}
+// Clone will copy the URL
+func (c *URL) Clone() *URL {
+ return c.CloneWithFilter(nil, nil)
+}
+
func (c *URL) RangeAttributes(f func(key string, value any) bool) {
c.attributesLock.RLock()
defer c.attributesLock.RUnlock()
@@ -897,19 +940,7 @@ 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.SetParam(key, value)
- }
- return true
- })
- return newURL
+ return c.CloneWithFilter(excludeParams, nil)
}
func (c *URL) Compare(comp cm.Comparator) int {
@@ -927,24 +958,7 @@ 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{}
- for _, reserveParam := range reserveParams {
- v := c.GetParam(reserveParam, "")
- if len(v) != 0 {
- params.Set(reserveParam, v)
- }
- }
-
- 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),
- )
+ return c.CloneWithFilter(nil, reserveParams)
}
// IsEquals compares if two URLs equals with each other. Excludes are all
parameter keys which should ignored.
diff --git a/common/url_test.go b/common/url_test.go
index 887ced1a3..8c04edd7c 100644
--- a/common/url_test.go
+++ b/common/url_test.go
@@ -33,6 +33,8 @@ package common
import (
"encoding/base64"
"net/url"
+ "strconv"
+ "sync"
"testing"
"time"
)
@@ -1374,3 +1376,48 @@ func TestAppendParamWithEmptyValue(t *testing.T) {
assert.Contains(t, name, "providers")
assert.Contains(t, name, "com.test.Service")
}
+
+func TestCloneThreadSafe(t *testing.T) {
+ u, err :=
NewURL("dubbo://127.0.0.1:20000/com.ikurento.user.UserProvider?interface=com.ikurento.user.UserProvider&group=&version=2.6.0&configVersion=1.0")
+ require.NoError(t, err)
+
+ var wg sync.WaitGroup
+ wg.Add(100)
+
+ // Reader goroutines
+ for i := 0; i < 50; i++ {
+ go func() {
+ defer wg.Done()
+ for j := 0; j < 100; j++ {
+ c := u.Clone()
+ _ = c.String() // Use the clone to avoid
optimization
+ }
+ }()
+ }
+
+ // Writer goroutines for params
+ for i := 0; i < 25; i++ {
+ go func(i int) {
+ defer wg.Done()
+ for j := 0; j < 100; j++ {
+ key := "key" + strconv.Itoa(j)
+ val := "val" + strconv.Itoa(i)
+ u.SetParam(key, val)
+ }
+ }(i)
+ }
+
+ // Writer goroutines for attributes
+ for i := 0; i < 25; i++ {
+ go func(i int) {
+ defer wg.Done()
+ for j := 0; j < 100; j++ {
+ key := "attr" + strconv.Itoa(j)
+ val := "attr_val" + strconv.Itoa(i)
+ u.SetAttribute(key, val)
+ }
+ }(i)
+ }
+
+ wg.Wait()
+}
diff --git a/go.mod b/go.mod
index 2517cd750..8c20a2481 100644
--- a/go.mod
+++ b/go.mod
@@ -30,7 +30,6 @@ require (
github.com/hashicorp/golang-lru v0.5.4
github.com/hashicorp/vault/sdk v0.7.0
github.com/influxdata/tdigest v0.0.1
- github.com/jinzhu/copier v0.3.5
github.com/knadh/koanf v1.5.0
github.com/magiconair/properties v1.8.5
github.com/mattn/go-colorable v0.1.13
diff --git a/go.sum b/go.sum
index 41e0e4992..733ebade4 100644
--- a/go.sum
+++ b/go.sum
@@ -473,8 +473,6 @@ github.com/influxdata/influxdb1-client
v0.0.0-20191209144304-8bf82d3c094d/go.mod
github.com/influxdata/tdigest v0.0.1
h1:XpFptwYmnEKUqmkcDjrzffswZ3nvNeevbUSLPP/ZzIY=
github.com/influxdata/tdigest v0.0.1/go.mod
h1:Z0kXnxzbTC2qrx4NaIzYkE1k66+6oEDQTvL95hQFh5Y=
github.com/jehiah/go-strftime v0.0.0-20171201141054-1d33003b3869/go.mod
h1:cJ6Cj7dQo+O6GJNiMx+Pa94qKj+TG8ONdKHgMNIyyag=
-github.com/jinzhu/copier v0.3.5 h1:GlvfUwHk62RokgqVNvYsku0TATCF7bAHVwEXoBh3iJg=
-github.com/jinzhu/copier v0.3.5/go.mod
h1:DfbEm0FYsaqBcKcFuvmOZb218JkPGtvSHsKg8S8hyyg=
github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod
h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
github.com/jmespath/go-jmespath v0.4.0
h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
github.com/jmespath/go-jmespath v0.4.0/go.mod
h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo=
