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.

Sign up for GitHub

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

Jump to bottom

Data race with gin-gonic v1.7.0 #2674

Closed
jerome-laforge opened this issue Apr 8, 2021 · 4 comments
Closed

Data race with gin-gonic v1.7.0 #2674

jerome-laforge opened this issue Apr 8, 2021 · 4 comments
Labels
bug
Milestone
v1.7.1

Comments

@jerome-laforge
Copy link
Contributor

jerome-laforge commented Apr 8, 2021 •
edited
Loading

With v1.7.0, a data race has been raised by our unit tests whith -race (not with previous version)

go version go1.16 linux/amd64

=== RUN   TestXXXX
==================
WARNING: DATA RACE
Write at 0x00c0003ce998 by goroutine 169:
  github.com/gin-gonic/gin.(*Context).RemoteIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:771 +0x1ac
  github.com/gin-gonic/gin.(*Context).ClientIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:740 +0xaf
  myproject/common/utils/log.JSONRecover()
      /home/jerome-laforge/dev/src/myproject/common/utils/log/ginRecover.go:23 +0x178
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0x106
  myproject/common/utils/log.JSONLog()
      /home/jerome-laforge/dev/src/myproject/common/utils/log/ginLog.go:24 +0x54
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xe1
  myproject/common/samplerlog.SetMiddleware.func1()
      /home/jerome-laforge/dev/src/myproject/common/samplerlog/samplerlog.go:30 +0x6c
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xac9
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:489 +0xa35
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:445 +0x376
  net/http.serverHandler.ServeHTTP()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2887 +0xca
  net/http.(*conn).serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:1952 +0x87d

Previous write at 0x00c0003ce998 by goroutine 41:
  github.com/gin-gonic/gin.(*Context).RemoteIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:771 +0x1ac
  github.com/gin-gonic/gin.(*Context).ClientIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:740 +0xaf
  myproject/common/utils/log.JSONRecover()
      /home/jerome-laforge/dev/src/myproject/common/utils/log/ginRecover.go:23 +0x178
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0x106
  myproject/common/utils/log.JSONLog()
      /home/jerome-laforge/dev/src/myproject/common/utils/log/ginLog.go:24 +0x54
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xe1
  myproject/common/samplerlog.SetMiddleware.func1()
      /home/jerome-laforge/dev/src/myproject/common/samplerlog/samplerlog.go:30 +0x6c
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xac9
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:489 +0xa35
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:445 +0x376
  net/http.serverHandler.ServeHTTP()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2887 +0xca
  net/http.(*conn).serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:1952 +0x87d

Goroutine 169 (running) created at:
  net/http.(*Server).Serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3013 +0x644
  net/http/httptest.(*Server).goServe.func1()
      /home/jerome-laforge/sdk/go1.16/src/net/http/httptest/server.go:308 +0xd8

Goroutine 41 (running) created at:
  net/http.(*Server).Serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3013 +0x644
  net/http/httptest.(*Server).goServe.func1()
      /home/jerome-laforge/sdk/go1.16/src/net/http/httptest/server.go:308 +0xd8
@jerome-laforge
Copy link
Contributor Author

can be reproduce with this simple test:

package test

import (
	"net/http"
	"testing"

	"github.com/gin-gonic/gin"
)

func BenchmarkRace(b *testing.B) {
	r := gin.New()

	r.GET("/", func(ctx *gin.Context) {
		ctx.String(http.StatusOK, ctx.ClientIP())
	})

	go http.ListenAndServe(":8080", r)

	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {

			resp, err := http.Get("http://127.0.0.1:8080/")
			if err != nil {
				b.Error(err)
				continue
			}

			defer resp.Body.Close()

			if resp.StatusCode != http.StatusOK {
				b.Fail()
			}
		}
	})
}
[GIN-debug] GET    /                         --> BenchmarkRace.func1 (1 handlers)
==================
WARNING: DATA RACE
Write at 0x00c0000ba178 by goroutine 67:
  github.com/gin-gonic/gin.(*Context).RemoteIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:771 +0x1ac
  github.com/gin-gonic/gin.(*Context).ClientIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:740 +0xaf
  test.BenchmarkRace.func1()
      /home/jerome-laforge/dev/src/test/toto_test.go:14 +0x3c
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xac9
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:489 +0xa35
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:445 +0x376
  net/http.serverHandler.ServeHTTP()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2887 +0xca
  net/http.(*conn).serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:1952 +0x87d

Previous write at 0x00c0000ba178 by goroutine 62:
  github.com/gin-gonic/gin.(*Context).RemoteIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:771 +0x1ac
  github.com/gin-gonic/gin.(*Context).ClientIP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:740 +0xaf
  test.BenchmarkRace.func1()
      /home/jerome-laforge/dev/src/test/toto_test.go:14 +0x3c
  github.com/gin-gonic/gin.(*Context).Next()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/context.go:165 +0xac9
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:489 +0xa35
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/jerome-laforge/go/pkg/mod/github.com/gin-gonic/[email protected]/gin.go:445 +0x376
  net/http.serverHandler.ServeHTTP()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2887 +0xca
  net/http.(*conn).serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:1952 +0x87d

Goroutine 67 (running) created at:
  net/http.(*Server).Serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3013 +0x644
  net/http.(*Server).ListenAndServe()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2910 +0x109
  net/http.ListenAndServe()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3164 +0xf4

Goroutine 62 (running) created at:
  net/http.(*Server).Serve()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3013 +0x644
  net/http.(*Server).ListenAndServe()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:2910 +0x109
  net/http.ListenAndServe()
      /home/jerome-laforge/sdk/go1.16/src/net/http/server.go:3164 +0xf4
==================
    benchmark.go:198: race detected during execution of benchmark
--- FAIL: BenchmarkRace-8
    benchmark.go:198: race detected during execution of benchmark
FAIL

Process finished with the exit code 1

@jerome-laforge jerome-laforge changed the title Data race with gin-gonic 1.7.0 Data race with gin-gonic v1.7.0 Apr 8, 2021
@fifsky
Copy link
Contributor

fifsky commented Apr 8, 2021

https://github.com/gin-gonic/gin/blob/master/context.go#L771
This line seems unnecessary

@appleboy appleboy modified the milestones: v1.7, v1.7.1 Apr 8, 2021
@appleboy appleboy added the bug label Apr 8, 2021
@appleboy appleboy mentioned this issue Apr 8, 2021
Merged
2 tasks
appleboy added a commit that referenced this issue Apr 8, 2021
@fifsky @appleboy
fix: data race with trustedCIDRs (#2674) (#2675)
03e5e05 
Co-authored-by: Bo-Yi Wu <[email protected]>
@appleboy appleboy removed this from the v1.7.1 milestone Apr 8, 2021
@appleboy
Copy link
Member

appleboy commented Apr 8, 2021

Fixed in #2675

@appleboy appleboy closed this as completed Apr 8, 2021
@appleboy
Copy link
Member

appleboy commented Apr 8, 2021

bump to v1.7.1 version

@thinkerou thinkerou added this to the v1.7.1 milestone Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v1.7.1
Development

No branches or pull requests
4 participants
@appleboy @fifsky @thinkerou @jerome-laforge