search

gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
77.93k stars 7.97k forks source link

Data race WARNING #1180

Closed mymtw closed 4 months ago

mymtw commented 6 years ago

Hi, I'm setting gin mode

    gin.SetMode("test")
    r := gin.Default()

And when I'm running my tests in t.Parallel() with flag -race -> I'm getting data race warning:

==================
WARNING: DATA RACE
Write at 0x000005c34cb8 by goroutine 38:
  gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin.SetMode()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin/mode.go:56 +0x1fe
  gitea.io/atlantide/curiosity/config/routes.GinRouter()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/config/routes/routes.go:33 +0x2ba
  gitea.io/atlantide/curiosity/test/controllers.(*SessionsControllerSuite).TestCreate()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/sessions_controller_test.go:124 +0xec6
  runtime.call32()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/runtime/asm_amd64.s:514 +0x47
  reflect.Value.Call()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/reflect/value.go:302 +0xc0
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run.func2()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:102 +0x31f
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Previous write at 0x000005c34cb8 by goroutine 44:
  gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin.SetMode()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin/mode.go:56 +0x1fe
  gitea.io/atlantide/curiosity/config/routes.GinRouter()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/config/routes/routes.go:33 +0x2ba
  gitea.io/atlantide/curiosity/test/controllers.(*UsersControllerSuite).TestCreate()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/users_controller_test.go:164 +0xc9d
  runtime.call32()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/runtime/asm_amd64.s:514 +0x47
  reflect.Value.Call()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/reflect/value.go:302 +0xc0
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run.func2()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:102 +0x31f
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Goroutine 38 (running) created at:
  testing.(*T).Run()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:697 +0x543
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.runTests()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:121 +0xd6
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:108 +0x709
  gitea.io/atlantide/curiosity/test/controllers.TestSessionsControllerSuite()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/sessions_controller_test.go:236 +0x6c
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Goroutine 44 (running) created at:
  testing.(*T).Run()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:697 +0x543
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.runTests()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:121 +0xd6
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:108 +0x709
  gitea.io/atlantide/curiosity/test/controllers.TestUsersControllerSuite()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/users_controller_test.go:489 +0x6c
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107
==================
==================
WARNING: DATA RACE
Write at 0x00000526da40 by goroutine 38:
  gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin.SetMode()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin/mode.go:60 +0x103
  gitea.io/atlantide/curiosity/config/routes.GinRouter()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/config/routes/routes.go:33 +0x2ba
  gitea.io/atlantide/curiosity/test/controllers.(*SessionsControllerSuite).TestCreate()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/sessions_controller_test.go:124 +0xec6
  runtime.call32()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/runtime/asm_amd64.s:514 +0x47
  reflect.Value.Call()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/reflect/value.go:302 +0xc0
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run.func2()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:102 +0x31f
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Previous write at 0x00000526da40 by goroutine 44:
  gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin.SetMode()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/gin-gonic/gin/mode.go:60 +0x103
  gitea.io/atlantide/curiosity/config/routes.GinRouter()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/config/routes/routes.go:33 +0x2ba
  gitea.io/atlantide/curiosity/test/controllers.(*UsersControllerSuite).TestCreate()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/users_controller_test.go:164 +0xc9d
  runtime.call32()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/runtime/asm_amd64.s:514 +0x47
  reflect.Value.Call()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/reflect/value.go:302 +0xc0
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run.func2()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:102 +0x31f
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Goroutine 38 (running) created at:
  testing.(*T).Run()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:697 +0x543
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.runTests()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:121 +0xd6
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:108 +0x709
  gitea.io/atlantide/curiosity/test/controllers.TestSessionsControllerSuite()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/sessions_controller_test.go:236 +0x6c
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107

Goroutine 44 (running) created at:
  testing.(*T).Run()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:697 +0x543
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.runTests()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:121 +0xd6
  gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite.Run()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/vendor/github.com/stretchr/testify/suite/suite.go:108 +0x709
  gitea.io/atlantide/curiosity/test/controllers.TestUsersControllerSuite()
      /Users/apple/Developer/goproject/src/gitea.io/atlantide/curiosity/test/controllers/users_controller_test.go:489 +0x6c
  testing.tRunner()
      /Users/apple/.gimme/versions/go1.8.1.darwin.amd64/src/testing/testing.go:657 +0x107
==================

Maybe reason in yr code ginMode = testCode ?: 

func SetMode(value string) {
    switch value {
    case DebugMode:
        ginMode = debugCode                         <<<<<<<<<<< ?????
    case ReleaseMode:
        ginMode = releaseCode                         <<<<<<<<<<< ?????
    case TestMode:
        ginMode = testCode                                <<<<<<<<<<< ?????
    default:
        panic("gin mode unknown: " + value)
    }
    modeName = value                                      <<<<<<<<<<< ?????
}

Your are changing global var from function, it looks like this is not safe. The warning in this context is not dangerous, but it will be better to fix this ASAP

thinkerou commented 6 years ago

maybe you should post simple example code and how use it?

And I use one simple example includes gin.SetMode("test") and GET, and then use the following curl command:

while true
do
curl -v http://localhost:8080/test &
done

But, I don't reproduce the issue.

javierprovecho commented 6 years ago

SetMode() and other Gin init funcs are not supposed to be called concurrently. They where wrongly designed in origin, by allowing/using a global status. I recommend a simple wrap function for doing gin init router and stuff, with a mutex if you are using Parallel testing.

Changing this behavior would be considered a breaking change, so a proper discussion should be started.

javierprovecho commented 6 years ago

Another option could be to add a mutex to all global functions like SetMode(), this won't impact performance and should be transparent to all users.

mymtw commented 6 years ago

@thinkerou I meant another thing. testing - means test coverage for my code. Here is https://golang.org/pkg/testing/ @javierprovecho thank you, but I'm afraid that adding mutex inside parallel tests will not bring any profit from parallel execution

They where wrongly designed in origin, by allowing/using a global status.

true

javierprovecho commented 6 years ago

@mymtw I don't think so, I meant adding a mutex around SetMode(), the rest of the test should run fine in parallel 😉

cornelk commented 6 years ago

Why can't the ginMode be read and set atomically? No need to use a mutex here. The changes needed are minimal.