Fix context.Params race condition on Copy()

[samuelabreu]

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]

Codecov Report

Merging #1841 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
+ Coverage   98.74%   98.75%   +<.01%     
==========================================
  Files          38       38              
  Lines        2157     2160       +3     
==========================================
+ Hits         2130     2133       +3     
  Misses         15       15              
  Partials       12       12

Impacted Files	Coverage Δ
context.go	98.38% <100%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35e33d3...0466028. Read the comment docs.

[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 the race condition on master perfectly. 👍

[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 and deleted your fix :) and added all 3 versions of the test into the Travis...

The good news is all 3 seem to fail 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()
}

[samuelabreu]

@dmarkham, this version looks way better

Im on a Mac, maybe it is something related to the OS scheduler running one after another on my setup and on travis (linux) running at the same time.

I will update my branch, sync with upstream as soon as possible, thank you.

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