[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #775: fix url.params fatal error: concurrent map read and map write

[GitBox <gi...@apache.org>]

AlexStocks commented on a change in pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#discussion_r493454613



##########
File path: common/url.go
##########
@@ -393,34 +396,33 @@ func (c URL) Service() string {
 }
 
 // AddParam will add the key-value pair
-// Not thread-safe
-// think twice before using it.
 func (c *URL) AddParam(key string, value string) {
+	c.paramsLock.RLock()
+	defer c.paramsLock.RUnlock()
 	c.params.Add(key, value)
 }
 
 // AddParamAvoidNil will add key-value pair
-// Not thread-safe
-// think twice before using it.
 func (c *URL) AddParamAvoidNil(key string, value string) {
 	if c.params == nil {
 		c.params = url.Values{}
 	}
 
-	c.params.Add(key, value)
+	c.AddParam(key, value)
 }
 
 // SetParam will put the key-value pair into url
-// it's not thread safe.
-// think twice before you want to use this method
 // usually it should only be invoked when you want to initialized an url
 func (c *URL) SetParam(key string, value string) {
+	c.paramsLock.RLock()
+	defer c.paramsLock.RUnlock()
 	c.params.Set(key, value)
 }
 
 // RangeParams will iterate the params
-// it's not thread-safe
 func (c *URL) RangeParams(f func(key, value string) bool) {

Review comment:
       u should exec the @f outside the lock to limit the lock scope.
   
   ```Go
           var (
               flag bool
               key string
               value string
           )
           func() {
           c.paramsLock.RLock()
   	defer c.paramsLock.RUnlock()
   	   for k, v := range c.params {
   		if !f(k, v[0]) {
                            key = k
                            value = v[0]
                            flag = true
   			break
   		}
   	  }
           }()
           if flag {
             f(key, value) 
           }
   ```

##########
File path: common/url.go
##########
@@ -393,34 +396,33 @@ func (c URL) Service() string {
 }
 
 // AddParam will add the key-value pair
-// Not thread-safe
-// think twice before using it.
 func (c *URL) AddParam(key string, value string) {
+	c.paramsLock.RLock()
+	defer c.paramsLock.RUnlock()
 	c.params.Add(key, value)
 }
 
 // AddParamAvoidNil will add key-value pair
-// Not thread-safe
-// think twice before using it.
 func (c *URL) AddParamAvoidNil(key string, value string) {
 	if c.params == nil {
 		c.params = url.Values{}
 	}
 
-	c.params.Add(key, value)
+	c.AddParam(key, value)
 }
 
 // SetParam will put the key-value pair into url
-// it's not thread safe.
-// think twice before you want to use this method
 // usually it should only be invoked when you want to initialized an url
 func (c *URL) SetParam(key string, value string) {
+	c.paramsLock.RLock()

Review comment:
       Hey, guy, pls using write lock instead.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org