You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2020/09/23 10:04:21 UTC

[GitHub] [dubbo-go] jack15083 opened a new pull request #775: fix url.params fatal error: concurrent map read and map write

jack15083 opened a new pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775


   **What this PR does**:
   fix url.params fatal error: concurrent map read and map write
   **Which issue(s) this PR fixes**:
   https://github.com/apache/dubbo-go/issues/772
   


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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
jack15083 commented on a change in pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#discussion_r493994614



##########
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:
       test this code, u should remove if flag {f(key, value)}




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

[GitHub] [dubbo-go] georgehao commented on pull request #775: Fix: url.params fatal error: concurrent map read and map write

Posted by GitBox <gi...@apache.org>.
georgehao commented on pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#issuecomment-698073378


   pls see #511 , because many url param of function is copy, just add rwmutex may appear warnings. 
   
   


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

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

Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#discussion_r493995106



##########
File path: common/url.go
##########
@@ -393,43 +396,59 @@ 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.Lock()
+	defer c.paramsLock.Unlock()
 	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)

Review comment:
       I think the write lock should cover all line of this method




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

[GitHub] [dubbo-go] codecov-commenter commented on pull request #775: Fix: url.params fatal error: concurrent map read and map write

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#issuecomment-698075169


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=h1) Report
   > Merging [#775](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/dubbo-go/commit/b9f1f12965c3426a39cd70d183b5a9be8c1018fa?el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `80.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/775/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #775      +/-   ##
   ===========================================
   - Coverage    59.84%   59.78%   -0.06%     
   ===========================================
     Files          259      259              
     Lines        12722    12732      +10     
   ===========================================
   - Hits          7613     7612       -1     
   - Misses        4156     4170      +14     
   + Partials       953      950       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [common/url.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-Y29tbW9uL3VybC5nbw==) | `61.48% <80.00%> (+0.64%)` | :arrow_up: |
   | [metadata/report/delegate/delegate\_report.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-bWV0YWRhdGEvcmVwb3J0L2RlbGVnYXRlL2RlbGVnYXRlX3JlcG9ydC5nbw==) | `29.66% <0.00%> (-9.33%)` | :arrow_down: |
   | [cluster/cluster\_impl/base\_cluster\_invoker.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYmFzZV9jbHVzdGVyX2ludm9rZXIuZ28=) | `57.57% <0.00%> (-9.10%)` | :arrow_down: |
   | [config\_center/nacos/client.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-Y29uZmlnX2NlbnRlci9uYWNvcy9jbGllbnQuZ28=) | `67.36% <0.00%> (+2.10%)` | :arrow_up: |
   | [filter/filter\_impl/hystrix\_filter.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-ZmlsdGVyL2ZpbHRlcl9pbXBsL2h5c3RyaXhfZmlsdGVyLmdv) | `70.19% <0.00%> (+2.88%)` | :arrow_up: |
   | [config\_center/nacos/facade.go](https://codecov.io/gh/apache/dubbo-go/pull/775/diff?src=pr&el=tree#diff-Y29uZmlnX2NlbnRlci9uYWNvcy9mYWNhZGUuZ28=) | `89.65% <0.00%> (+10.34%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=footer). Last update [b9f1f12...ccd1881](https://codecov.io/gh/apache/dubbo-go/pull/775?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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

Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775#discussion_r493987590



##########
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:
       Agree




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

[GitHub] [dubbo-go] AlexStocks merged pull request #775: Fix: url.params fatal error: concurrent map read and map write

Posted by GitBox <gi...@apache.org>.
AlexStocks merged pull request #775:
URL: https://github.com/apache/dubbo-go/pull/775


   


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