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

fix: data race in seriesIterator, use atomic int #12223

Merged
merged 2 commits into from
Mar 15, 2024
Merged

fix: data race in seriesIterator, use atomic int #12223

merged 2 commits into from
Mar 15, 2024

Conversation

poyzannur
Copy link
Contributor

Before the fix

go test --race ./pkg/logql/...
?   	github.com/grafana/loki/pkg/logql/vector	[no test files]
==================
WARNING: DATA RACE
Read at 0x00c0009d4bc0 by goroutine 340:
  github.com/grafana/loki/pkg/iter.(*seriesIterator).Next()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:576 +0x2c
  github.com/grafana/loki/pkg/iter.(*sortSampleIterator).init()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:387 +0xec
  github.com/grafana/loki/pkg/iter.(*sortSampleIterator).Next()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:405 +0x2c
  github.com/grafana/loki/pkg/iter.NewPeekingSampleIterator()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:45 +0x78
  github.com/grafana/loki/pkg/logql.(*DefaultEvaluator).NewStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:272 +0x6f0
  github.com/grafana/loki/pkg/logql.newBinOpStepEvaluator.func1()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:698 +0xa8
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/poyzannur/workspace/loki/vendor/golang.org/x/sync/errgroup/errgroup.go:75 +0x6c

Previous write at 0x00c0009d4bc0 by goroutine 341:
  github.com/grafana/loki/pkg/iter.(*seriesIterator).Next()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:576 +0x3c
  github.com/grafana/loki/pkg/iter.(*sortSampleIterator).init()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:387 +0xec
  github.com/grafana/loki/pkg/iter.(*sortSampleIterator).Next()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:405 +0x2c
  github.com/grafana/loki/pkg/iter.NewPeekingSampleIterator()
      /Users/poyzannur/workspace/loki/pkg/iter/sample_iterator.go:45 +0x78
  github.com/grafana/loki/pkg/logql.(*DefaultEvaluator).NewStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:272 +0x6f0
  github.com/grafana/loki/pkg/logql.newBinOpStepEvaluator.func2()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:706 +0xb4
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/poyzannur/workspace/loki/vendor/golang.org/x/sync/errgroup/errgroup.go:75 +0x6c

Goroutine 340 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /Users/poyzannur/workspace/loki/vendor/golang.org/x/sync/errgroup/errgroup.go:72 +0x10c
  github.com/grafana/loki/pkg/logql.newBinOpStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:696 +0x598
  github.com/grafana/loki/pkg/logql.(*DefaultEvaluator).NewStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:274 +0x798
  github.com/grafana/loki/pkg/logql.(*query).evalSample()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:348 +0x21c
  github.com/grafana/loki/pkg/logql.(*query).Eval()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:285 +0x4d4
  github.com/grafana/loki/pkg/logql.(*query).Exec()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:252 +0x118c
  github.com/grafana/loki/pkg/logql.TestStepEvaluator_Error.func1()
      /Users/poyzannur/workspace/loki/pkg/logql/engine_test.go:2448 +0x4b0
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1648 +0x40

Goroutine 341 (finished) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /Users/poyzannur/workspace/loki/vendor/golang.org/x/sync/errgroup/errgroup.go:72 +0x10c
  github.com/grafana/loki/pkg/logql.newBinOpStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:704 +0x738
  github.com/grafana/loki/pkg/logql.(*DefaultEvaluator).NewStepEvaluator()
      /Users/poyzannur/workspace/loki/pkg/logql/evaluator.go:274 +0x798
  github.com/grafana/loki/pkg/logql.(*query).evalSample()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:348 +0x21c
  github.com/grafana/loki/pkg/logql.(*query).Eval()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:285 +0x4d4
  github.com/grafana/loki/pkg/logql.(*query).Exec()
      /Users/poyzannur/workspace/loki/pkg/logql/engine.go:252 +0x118c
  github.com/grafana/loki/pkg/logql.TestStepEvaluator_Error.func1()
      /Users/poyzannur/workspace/loki/pkg/logql/engine_test.go:2448 +0x4b0
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1648 +0x40
==================
--- FAIL: TestStepEvaluator_Error (0.00s)
    --- FAIL: TestStepEvaluator_Error/binOpStepEvaluator (0.00s)
        testing.go:1465: race detected during execution of test
    testing.go:1465: race detected during execution of test
FAIL
FAIL	github.com/grafana/loki/pkg/logql	129.372s
ok  	github.com/grafana/loki/pkg/logql/log	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/jsonexpr	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/logfmt	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/pattern	(cached)
ok  	github.com/grafana/loki/pkg/logql/sketch	(cached)
ok  	github.com/grafana/loki/pkg/logql/syntax	(cached)

After the fix

go test --race ./pkg/logql/...                   
?   	github.com/grafana/loki/pkg/logql/vector	[no test files]
ok  	github.com/grafana/loki/pkg/logql	(cached)
ok  	github.com/grafana/loki/pkg/logql/log	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/jsonexpr	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/logfmt	(cached)
ok  	github.com/grafana/loki/pkg/logql/log/pattern	(cached)
ok  	github.com/grafana/loki/pkg/logql/sketch	(cached)
ok  	github.com/grafana/loki/pkg/logql/syntax	(cached)

@poyzannur poyzannur requested a review from a team as a code owner March 15, 2024 10:26
@poyzannur poyzannur changed the title [chore] Use atomic int in the seriesIterator fix: Use atomic int in the seriesIterator Mar 15, 2024
@poyzannur poyzannur changed the title fix: Use atomic int in the seriesIterator fix: data race in seriesIterator, use atomic int Mar 15, 2024
MichelHollands
MichelHollands approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

@poyzannur poyzannur merged commit 60dcee1 into main Mar 15, 2024
11 checks passed
@poyzannur poyzannur deleted the poyzannur/fix-data-race-in-sample-iterator branch March 15, 2024 10:56
@poyzannur poyzannur mentioned this pull request Mar 15, 2024
Merged
@paul1r
Copy link
Collaborator

paul1r commented Mar 15, 2024

relates to: #8586

@bboreham
Copy link
Contributor

I suggest this is not the right approach. Making every seriesIterator atomic will have an impact on performance, and is at odds with the concept of an iterator. Iterators are analogous to an index into an array: they should not be accessed from multiple goroutines without some interlock at a higher level.

I think the race reported on this test arises because errorIteratorQuerier is a mock implementation that returns the same iterators every time it is called. If this is modified to return a new set of iterators each time, the race goes away.

bboreham added a commit to bboreham/loki that referenced this pull request Mar 18, 2024
@bboreham
Revert "fix: data race in seriesIterator, use atomic int (grafana#12223…
9faac32 
…)"

This reverts commit 60dcee1.
We don't want atomic iterators.
@bboreham bboreham mentioned this pull request Mar 18, 2024
Merged
2 tasks
@loki-gh-app loki-gh-app bot mentioned this pull request Mar 27, 2024
Closed
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
@poyzannur @rhnasc
fix: data race in seriesIterator, use atomic int (grafana#12223)
714a3e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@na-- na-- na-- left review comments

@MichelHollands MichelHollands MichelHollands approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@poyzannur @paul1r @bboreham @na-- @MichelHollands