Add fixes against race conditions by mjeshtri · Pull Request #22 · penglongli/gin-metrics

mjeshtri · 2022-01-19T15:05:45Z

Ensuring that the monitor is declared a single time by becoming atomic. This approach protects also against race conditions
make BloomFilter thread-safe
In addition adding GitHub Actions to automate testing. Under current setup, the tests are triggered whenever a PR is created against master branch or whenever commits are merged in master.

ensuring that the monitor is declared a single time by becoming atomic. This approach protects also against race conditions

…o master branch

hengfeiyang

in bloom filter, can we use RWMutex?
can you add some bennchmarks?

hengfeiyang · 2022-01-27T08:25:56Z

bloom/bloom.go

 var seeds = []uint{7, 11, 13, 31, 37, 61}

 type BloomFilter struct {
+	mu    sync.Mutex


this can use RWMutex, then bf.Contains can use bf.mu.RLock()

I ran some benchmarks, and these are the results:

Current implementation (sync.Mutex):

goos: linux goarch: amd64 pkg: github.com/penglongli/gin-metrics/ginmetrics cpu: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz BenchmarkMiddlewareSamePort-4 10000 187130 ns/op BenchmarkMiddlewareSamePort-4 10000 368713 ns/op BenchmarkMiddlewareSamePort-4 10000 376937 ns/op BenchmarkMiddlewareSamePort-4 10000 360904 ns/op BenchmarkMiddlewareSamePort-4 10000 178943 ns/op PASS ok github.com/penglongli/gin-metrics/ginmetrics 19.239s

Suggesteg implementation (sync.RWMutex, where bf.Contains() is protected by bf.mu.RLock()):

goos: linux goarch: amd64 pkg: github.com/penglongli/gin-metrics/ginmetrics cpu: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz BenchmarkMiddlewareSamePort-4 10000 179478 ns/op BenchmarkMiddlewareSamePort-4 10000 364757 ns/op BenchmarkMiddlewareSamePort-4 10000 365689 ns/op BenchmarkMiddlewareSamePort-4 10000 180693 ns/op BenchmarkMiddlewareSamePort-4 9828 360628 ns/op PASS ok github.com/penglongli/gin-metrics/ginmetrics 18.937s

Benchtest comparison:

name old time/op new time/op delta MiddlewareSamePort-4 295µs ±39% 290µs ±38% ~ (p=0.690 n=5+5)

As it can be seen the results are statistically indistinguishable. Increased the number of times benchmarks were run up to 25, and the result is still the same.

name old time/op new time/op delta MiddlewareSamePort-4 278µs ±38% 264µs ±45% ~ (p=0.658 n=25+25)

I suggest to keep it the way it is.

penglongli · 2022-02-08T08:37:14Z

@mjeshtri @hengfeiyang Sorry for the late reply, I'm working on this PR now

penglongli · 2022-02-08T09:34:06Z

Hi @mjeshtri

bitset is based on slice. A fixed-length slice will be created during the initialization of bitset. bitset.go#L121
When inserting a value, bitset will operate on the specified subscript. bitset.go#L174

So I think it's actually a Thread-Safe Set.

And i try to create a lot of goroutines concurrently to set the same value, it's okay.

# Conflicts: # bloom/bloom.go

mjeshtri · 2022-02-08T14:01:03Z

Hi @penglongli ,

Hi @mjeshtri
bitset is based on slice. A fixed-length slice will be created during the initialization of bitset. bitset.go#L121
When inserting a value, bitset will operate on the specified subscript. bitset.go#L174
So I think it's actually a Thread-Safe Set.
And i try to create a lot of goroutines concurrently to set the same value, it's okay.

This is interesting... If I remove the mutex protection in

gin-metrics/bloom/bloom.go

Lines 12 to 13 in 5dea4b6

    
           type BloomFilter struct { 
        
           	mu    sync.Mutex

bloom, the tests fail due to data racing. The based on the error message it is clear where it is coming from, the Add() and Contains() method of BloomFilter (i.e. (*BitSet).Set() and (*BitSet).Test()):

go test -v -race ./...
?       github.com/penglongli/gin-metrics/bloom [no test files]
=== RUN   TestSingletonRacing
--- PASS: TestSingletonRacing (0.05s)
=== RUN   TestMiddlewareSamePort
==================
WARNING: DATA RACE
Write at 0x00c0008763b8 by goroutine 120:
  github.com/bits-and-blooms/bitset.(*BitSet).Set()
      /home/mjeshtri/go/pkg/mod/github.com/bits-and-blooms/bitset@v1.2.0/bitset.go:174 +0x1b9
  github.com/penglongli/gin-metrics/bloom.(*BloomFilter).Add()
      /home/mjeshtri/gin-metrics/bloom/bloom.go:27 +0xd8
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).ginMetricHandle()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:126 +0x204
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:114 +0x3c6
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor-fm()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:103 +0x44
  github.com/gin-gonic/gin.(*Context).Next()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/context.go:165 +0xae3
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:489 +0x710
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:445 +0x3a5
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2878 +0x89a
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1929 +0x12e4
  net/http.(*Server).Serve·dwrap·82()
      /usr/local/go/src/net/http/server.go:3033 +0x58

Previous read at 0x00c0008763b8 by goroutine 58:
  github.com/bits-and-blooms/bitset.(*BitSet).Test()
      /home/mjeshtri/go/pkg/mod/github.com/bits-and-blooms/bitset@v1.2.0/bitset.go:163 +0x1c4
  github.com/penglongli/gin-metrics/bloom.(*BloomFilter).Contains()
      /home/mjeshtri/gin-metrics/bloom/bloom.go:37 +0xc4
github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).ginMetricHandle()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:125 +0x1ce
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:114 +0x3c6
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor-fm()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:103 +0x44
  github.com/gin-gonic/gin.(*Context).Next()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/context.go:165 +0xae3
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:489 +0x710
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:445 +0x3a5
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2878 +0x89a
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1929 +0x12e4
  net/http.(*Server).Serve·dwrap·82()
      /usr/local/go/src/net/http/server.go:3033 +0x58

Goroutine 120 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3033 +0x847
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:308 +0xc4

Goroutine 58 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3033 +0x847
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:308 +0xc4
==================
==================
WARNING: DATA RACE
Read at 0x00c0005de250 by goroutine 120:
  github.com/bits-and-blooms/bitset.(*BitSet).Set()
      /home/mjeshtri/go/pkg/mod/github.com/bits-and-blooms/bitset@v1.2.0/bitset.go:174 +0x189
  github.com/penglongli/gin-metrics/bloom.(*BloomFilter).Add()
      /home/mjeshtri/gin-metrics/bloom/bloom.go:27 +0xd8
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).ginMetricHandle()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:126 +0x204
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:114 +0x3c6
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor-fm()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:103 +0x44
  github.com/gin-gonic/gin.(*Context).Next()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/context.go:165 +0xae3
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:489 +0x710
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:445 +0x3a5
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2878 +0x89a
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1929 +0x12e4
  net/http.(*Server).Serve·dwrap·82()
      /usr/local/go/src/net/http/server.go:3033 +0x58

Previous write at 0x00c0005de250 by goroutine 58:
  github.com/bits-and-blooms/bitset.(*BitSet).Set()
      /home/mjeshtri/go/pkg/mod/github.com/bits-and-blooms/bitset@v1.2.0/bitset.go:174 +0x1b9
  github.com/penglongli/gin-metrics/bloom.(*BloomFilter).Add()
      /home/mjeshtri/gin-metrics/bloom/bloom.go:27 +0xd8
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).ginMetricHandle()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:126 +0x204
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:114 +0x3c6
  github.com/penglongli/gin-metrics/ginmetrics.(*Monitor).monitorInterceptor-fm()
      /home/mjeshtri/gin-metrics/ginmetrics/middleware.go:103 +0x44
  github.com/gin-gonic/gin.(*Context).Next()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/context.go:165 +0xae3
  github.com/gin-gonic/gin.(*Engine).handleHTTPRequest()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:489 +0x710
  github.com/gin-gonic/gin.(*Engine).ServeHTTP()
      /home/mjeshtri/go/pkg/mod/github.com/gin-gonic/gin@v1.7.4/gin.go:445 +0x3a5
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2878 +0x89a
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1929 +0x12e4
  net/http.(*Server).Serve·dwrap·82()
      /usr/local/go/src/net/http/server.go:3033 +0x58

Goroutine 120 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3033 +0x847
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:308 +0xc4

Goroutine 58 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3033 +0x847
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:308 +0xc4
==================
    testing.go:1152: race detected during execution of test
--- FAIL: TestMiddlewareSamePort (1.63s)
=== RUN   TestMiddlewareDifferentPort
--- PASS: TestMiddlewareDifferentPort (2.48s)
=== CONT  
    testing.go:1152: race detected during execution of test
FAIL
FAIL    github.com/penglongli/gin-metrics/ginmetrics    4.280s
FAIL
make: *** [Makefile:2: test] Error 1

penglongli · 2022-02-09T08:24:05Z

Hi @mjeshtri Because slice is not thread-safe, so the DataRace is detected.

But I don't know if I can ignore that, so i create a post here: https://groups.google.com/g/golang-nuts/c/ai0eQtnas7A

mjeshtri added 3 commits January 19, 2022 15:46

protect global monitor from race conditions

22b9f43

ensuring that the monitor is declared a single time by becoming atomic. This approach protects also against race conditions

add github actions to automatically run tests on PR or when pushing t…

20ede88

…o master branch

add tests status reporting badge

e2af5de

mjeshtri force-pushed the singleton branch from 125e399 to e2af5de Compare January 21, 2022 10:06

mjeshtri added 4 commits January 21, 2022 11:09

setup go

f1a43d5

wait until all goroutines are finished

19a6f34

make BloomFilter thread safe

86c4d6d

add tests for detecting data racing

f784a81

mjeshtri changed the title ~~Protect global monitor from race conditions~~ Add fixes against race conditions Jan 21, 2022

hengfeiyang reviewed Jan 27, 2022

View reviewed changes

Merge branch 'master' into singleton

5dea4b6

# Conflicts: # bloom/bloom.go

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Add fixes against race conditions#22

Add fixes against race conditions#22
mjeshtri wants to merge 8 commits intopenglongli:masterfrom
mjeshtri:singleton

mjeshtri commented Jan 19, 2022 •

edited

Loading

Uh oh!

hengfeiyang left a comment

Uh oh!

hengfeiyang Jan 27, 2022

Uh oh!

mjeshtri Jan 30, 2022

Uh oh!

penglongli commented Feb 8, 2022

Uh oh!

penglongli commented Feb 8, 2022

Uh oh!

mjeshtri commented Feb 8, 2022

Uh oh!

penglongli commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

mjeshtri commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hengfeiyang left a comment

Choose a reason for hiding this comment

Uh oh!

hengfeiyang Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

mjeshtri Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

penglongli commented Feb 8, 2022

Uh oh!

penglongli commented Feb 8, 2022

Uh oh!

mjeshtri commented Feb 8, 2022

Uh oh!

penglongli commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjeshtri commented Jan 19, 2022 •

edited

Loading