Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix context.Params race condition on Copy() #1841

Merged
merged 4 commits into from
May 27, 2019

Conversation

samuelabreu
Copy link
Contributor

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.

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
@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

dmarkham commented Apr 4, 2019

@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
Copy link
Contributor Author

@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
Copy link
Contributor

dmarkham commented Apr 5, 2019

@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
Copy link
Contributor Author

@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
Copy link
Contributor

dmarkham commented Apr 5, 2019

@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
Copy link
Contributor

dmarkham commented Apr 9, 2019

@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
Copy link
Contributor Author

@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
Copy link
Contributor

@appleboy @javierprovecho @thinkerou This Looks like a pretty sold PR. Would one of you have a second for some feedback please.

@appleboy
Copy link
Member

LGTM. Waiting for @thinkerou or @javierprovecho

@appleboy
Copy link
Member

@samuelabreu @dangkaka Can you add some benchmark ?

@dmarkham
Copy link
Contributor

dmarkham commented May 8, 2019

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

@appleboy appleboy added this to the 1.5 milestone May 26, 2019
@appleboy appleboy added the bug label May 26, 2019
@thinkerou thinkerou merged commit 6e320c9 into gin-gonic:master May 27, 2019
ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
* 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

* Using waitgroup to wait asynchronous test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants