Fix data race in ingester #2327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2327 +/- ##
=======================================
Coverage 60.99% 61.00%
=======================================
Files 158 158
Lines 12751 12754 +3
=======================================
+ Hits 7778 7781 +3
- Misses 4390 4391 +1
+ Partials 583 582 -1
|
pkg/ingester/flush.go
Outdated
| if err := c.Encode(); err != nil { | ||
| return err | ||
| } | ||
| streamsMtx.Unlock() |
There was a problem hiding this comment.
I think you should do a defer here.
There was a problem hiding this comment.
Can you explain who is racing for the stream data here ? Querying ?
There was a problem hiding this comment.
I think we should not defer Unlock. The reason being putting chunks to store here would be locked and other flush threads won't be able to call put until the lock is released. This would become sequential io and eventually make all flush threads sequential.
Line 337 in e22f365
There was a problem hiding this comment.
If you return an error you're deadlocked.
There was a problem hiding this comment.
Can you explain who is racing for the stream data here ? Querying ?
Yeah, it is querying which access the stream chunks here
Line 261 in e22f365
but this is Read protected with streamMtx lock here
Line 316 in e22f365
since c.Encode is not write locked it causes the race.
Also, reading first chunk of the stream and getting bounds on that causes the race.
Line 174 in e22f365
There are other places which will cause the race. All those are already protected with streamMtx but c.Encode was not protected.
There was a problem hiding this comment.
firstTime, _ := stream.chunks[0].chunk.Bounds() ? this one is fixed or already ok ?
There was a problem hiding this comment.
If you return an error you're deadlocked
oh yeah. You are right. Didn't realize that.
There was a problem hiding this comment.
firstTime, _ := stream.chunks[0].chunk.Bounds() ? this one is fixed or already ok ?
this was already protected
adityacs
force-pushed
the
ingester_data_race_fix
branch
from
c0740d3
to
4a8ee6d
Compare
What this PR does / why we need it:
Fixes data race in ingester
Which issue(s) this PR fixes:
Fixes #2265