Repository: gin-gonic/gin
Pull request #1841 (2019-04-03 18:18:36)

Fix context.Params race condition on Copy()
Using context.Param(key) on a context.Copy inside a goroutine
may lead to incorrect value on a high load, where another request
overwrite a Param

- With pull requests:
  - Open your pull request against `master`
  - Your pull request should have no more than two commits, if not you should squash them.
  - It should pass all tests in the available continuous integrations systems such as TravisCI.
  - You should add/modify tests to cover your proposed code changes.
  - If your pull request contains a new feature, please document it on the README.



codecov[bot]: # [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/1841?src=pr&el=h1) Report
> Merging [#1841](https://codecov.io/gh/gin-gonic/gin/pull/1841?src=pr&el=desc) into [master](https://codecov.io/gh/gin-gonic/gin/commit/2e915f4e5083995154f65a600c86582b5396d02a?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/gin-gonic/gin/pull/1841/graphs/tree.svg?width=650&token=Qpqxhwery8&height=150&src=pr)](https://codecov.io/gh/gin-gonic/gin/pull/1841?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
+ Coverage   98.64%   98.64%   +<.01%     
==========================================
  Files          41       41              
  Lines        2144     2147       +3     
==========================================
+ Hits         2115     2118       +3     
  Misses         18       18              
  Partials       11       11
```


| [Impacted Files](https://codecov.io/gh/gin-gonic/gin/pull/1841?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [context.go](https://codecov.io/gh/gin-gonic/gin/pull/1841/diff?src=pr&el=tree#diff-Y29udGV4dC5nbw==) | `98.38% <100%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gin-gonic/gin/pull/1841?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/gin-gonic/gin/pull/1841?src=pr&el=footer). Last update [2e915f4...8709d89](https://codecov.io/gh/gin-gonic/gin/pull/1841?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

dmarkham: Related to #1391 that is still trying to get into a release.
dmarkham: @samuelabreu  would you mind converting your test case to use a `WaitGroup`? Something a little more like this.
```
func TestRaceParamsContextCopy(t *testing.T) {
	DefaultWriter = os.Stdout
	router := Default()
	nameGroup := router.Group("/:name")
	var wg sync.WaitGroup
	{
		nameGroup.GET("/api", func(c *Context) {
			wg.Add(1)
			go func(c *Context, param string) {
				defer wg.Done()
				assert.Equal(t, c.Param("name"), param)
			}(c.Copy(), c.Param("name"))
		})
	}
	performRequest(router, "GET", "/name1/api")
	performRequest(router, "GET", "/name2/api")
	wg.Wait()
}
```
I'm not sure about others but, sleeps in test cases is a silly pet peeve of mine. Thank you very much for this test case it  highlights perfectly the race condition on master. This test case should be added regardless of witch version of this code gets merged :+1:  
 
samuelabreu: @dmarkham, had to use 2 waitgroups, this is ok?

```
func TestRaceParamsContextCopy(t *testing.T) {
	DefaultWriter = os.Stdout
	router := Default()
	var mainWg sync.WaitGroup
	var name1Wg sync.WaitGroup
	mainWg.Add(1)
	name1Wg.Add(1)
	nameGroup := router.Group("/:name")
	{
		nameGroup.GET("/api", func(c *Context) {
			go func(c *Context, param string) {
				if param == "name1" {
					name1Wg.Wait()
				}
				assert.Equal(t, c.Param("name"), param)
				if param == "name2" {
					name1Wg.Done()
				} else {
					mainWg.Done()
				}
			}(c.Copy(), c.Param("name"))
		})
	}
	performRequest(router, "GET", "/name1/api")
	performRequest(router, "GET", "/name2/api")
	mainWg.Wait()
}
```

dmarkham: @samuelabreu  Did you end up finding a edge case my version was not catching? I thought I had tested it well! So my question is why do you need 2?
samuelabreu: @dmarkham , when i tried it never fails the test.

I think they run one goroutine after another, and to cause a race condition, the first assert must be executed after the second request. 
dmarkham: > @dmarkham , when i tried it never fails the test.
> 
> I think they run one goroutine after another, and to cause a race condition, the first assert must be executed after the second request.
ok sorry I was sure I had something that was "clean" and "simple". I'll try to figure out what I was missing. I guess you should submit the version you like best at this point, I'm less excited about the `waitgroup` complexity now :(. Thank you for humoring me.

dmarkham: @samuelabreu So for fun... I took your [branch](https://github.com/dmarkham/gin/blob/054c013bf87a4a7f4031d362396ae3d3ecabf3a8/context_test.go#L1818-L1880) and deleted your fix :)   and added all 3 versions of the test into the Travis... 

The good news is all 3  seem to [fail](https://travis-ci.org/dmarkham/gin/jobs/517614895#L769) in Travis!
But it does not matter I'm pretty sure your right. I'm just getting lucky in the order of the run.

Maybe you would consider this hybrid approach:
```
func TestRaceParamsContextCopyWaitGroup(t *testing.T) {
	DefaultWriter = os.Stdout
	router := Default()
	nameGroup := router.Group("/:name")
	var wg sync.WaitGroup
	{
		nameGroup.GET("/api", func(c *Context) {
			wg.Add(1)
			go func(c *Context, param string) {
				defer wg.Done()
				// First assert must be executed after the second request
                                time.Sleep(50 * time.Millisecond)
				assert.Equal(t, c.Param("name"), param)
			}(c.Copy(), c.Param("name"))
		})
	}
	performRequest(router, "GET", "/name1/api")
	performRequest(router, "GET", "/name2/api")
	wg.Wait()
}
```




dmarkham: @appleboy  @javierprovecho  @thinkerou  This Looks like a pretty sold PR. Would one of you have a second for some feedback please.
appleboy: LGTM. Waiting for @thinkerou or @javierprovecho 
appleboy: @samuelabreu @dangkaka Can you add some benchmark ?
dmarkham: @appleboy  @thinkerou  I think this should get into `1.5.0` I can try to pull together a benchmarks, but the `Copy` code without this is just broken under load. People that use `Copy` I think would rather it work than be quick and sometimes work.

Patch:
diff --git a/context.go b/context.go
index 425b627..3f1f8ca 100644
--- a/context.go
+++ b/context.go
@@ -89,6 +89,9 @@ func (c *Context) Copy() *Context {
 	for k, v := range c.Keys {
 		cp.Keys[k] = v
 	}
+	paramCopy := make([]Param, len(cp.Params))
+	copy(paramCopy, cp.Params)
+	cp.Params = paramCopy
 	return &cp
 }
 


Test patch:
diff --git a/context_test.go b/context_test.go
index e8dcd3d..89cfb44 100644
--- a/context_test.go
+++ b/context_test.go
@@ -13,8 +13,10 @@ import (
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"reflect"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -1821,3 +1823,24 @@ func TestContextResetInHandler(t *testing.T) {
 		c.Next()
 	})
 }
+
+func TestRaceParamsContextCopy(t *testing.T) {
+	DefaultWriter = os.Stdout
+	router := Default()
+	nameGroup := router.Group("/:name")
+	var wg sync.WaitGroup
+	wg.Add(2)
+	{
+		nameGroup.GET("/api", func(c *Context) {
+			go func(c *Context, param string) {
+				defer wg.Done()
+				// First assert must be executed after the second request
+				time.Sleep(50 * time.Millisecond)
+				assert.Equal(t, c.Param("name"), param)
+			}(c.Copy(), c.Param("name"))
+		})
+	}
+	performRequest(router, "GET", "/name1/api")
+	performRequest(router, "GET", "/name2/api")
+	wg.Wait()
+}