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 data race in genny list Remove #3233

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Conversation

ryanhall07
Copy link
Collaborator

The data race is reading the value after the element has been put back
in the pool. Another goroutine can get the element out of pool before
the value is read again.

This was not causing an actual production issue because no caller
actually uses the returned value from Remove. However, it did cause
data race failures in tests when run with -race.

WARNING: DATA RACE
Write at 0x00c063a621d8 by goroutine 3582:
  github.com/m3db/m3/src/x/context.(*finalizeableList).insertValue()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/context/finalizeable_list_gen.go:202 +0xa8
  github.com/m3db/m3/src/x/context.(*finalizeableList).PushBack()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/context/finalizeable_list_gen.go:239 +0x96
  github.com/m3db/m3/src/x/context.(*ctx).registerFinalizeable()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/context/context.go:135 +0xce
  github.com/m3db/m3/src/x/context.(*ctx).RegisterFinalizer()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/context/context.go:109 +0xa2
  github.com/m3db/m3/src/dbnode/storage/index.(*block).aggregateWithSpan()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index/block.go:703 +0x690
  github.com/m3db/m3/src/dbnode/storage/index.(*block).Aggregate()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index/block.go:607 +0x20a
  github.com/m3db/m3/src/dbnode/storage.(*nsIndex).execBlockAggregateQueryFn()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index.go:1763 +0x611
  github.com/m3db/m3/src/dbnode/storage.(*nsIndex).execBlockAggregateQueryFn-fm()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index.go:1736 +0x150
  github.com/m3db/m3/src/dbnode/storage.(*nsIndex).queryWithSpan.func1()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index.go:1589 +0x22f
  github.com/m3db/m3/src/x/sync.(*workerPool).GoWithContext.func1()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/sync/worker_pool.go:128 +0x34
Previous read at 0x00c063a621d8 by goroutine 2940:
  [failed to restore the stack]
Goroutine 3582 (running) created at:
  github.com/m3db/m3/src/x/sync.(*workerPool).GoWithContext()
      /Users/rhall/go/src/github.com/m3db/m3/src/x/sync/worker_pool.go:127 +0x2e6
  github.com/m3db/m3/src/dbnode/storage.(*nsIndex).queryWithSpan()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index.go:1587 +0x8f2
  github.com/m3db/m3/src/dbnode/storage.(*nsIndex).query()
      /Users/rhall/go/src/github.com/m3db/m3/src/dbnode/storage/index.go:1467 +0x279

What this PR does / why we need it:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:


Does this PR require updating code package or user-facing documentation?:


The data race is reading the value after the element has been put back
in the pool. Another goroutine can get the element out of pool before
the value is read again.

This was not causing an actual production issue because no caller
actually uses the returned value from Remove. However, it did cause
data race failures in tests when run with -race.
@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #3233 (4d6f7a1) into master (d4ff5a6) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3233   +/-   ##
=======================================
  Coverage    72.3%    72.3%           
=======================================
  Files        1094     1094           
  Lines      101122   101123    +1     
=======================================
+ Hits        73190    73196    +6     
  Misses      22868    22868           
+ Partials     5064     5059    -5     
Flag Coverage Δ
aggregator 75.8% <ø> (-0.1%) ⬇️
cluster 84.8% <ø> (-0.1%) ⬇️
collector 84.3% <ø> (ø)
dbnode 78.8% <ø> (+<0.1%) ⬆️
m3em 74.4% <ø> (ø)
m3ninx 73.3% <ø> (-0.1%) ⬇️
metrics 20.0% <ø> (ø)
msg 74.2% <ø> (+0.3%) ⬆️
query 67.3% <ø> (ø)
x 80.2% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.


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 d4ff5a6...4d6f7a1. Read the comment docs.

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great to have this noise out of the way

@ryanhall07 ryanhall07 merged commit e31d6dc into master Feb 16, 2021
@ryanhall07 ryanhall07 deleted the rhall-genny-list-race branch February 16, 2021 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants