fix: Resolve some data races in chunk accesses #15080
Conversation
| @@ -239,8 +239,8 @@ func (s *streamIterator) Next() bool { | |||
| // remove the first stream | |||
| s.instances[0].streams = s.instances[0].streams[1:] | |||
|
|
|||
| stream.chunkMtx.RLock() | |||
| defer stream.chunkMtx.RUnlock() | |||
| stream.chunkMtx.Lock() | |||
There was a problem hiding this comment.
toWireChunk writes to c.blocks[*].offset on pkg/chunkenc/memchunk.go:674
Races with chunk.Bounds(), which doesn't strictly read this value, but does iterate over c.blocks.
There was a problem hiding this comment.
I briefly looked at memchunk.go to see if we could avoid the write to offset, but its not easy to do. Offset is expected to be set after encoding a chunk so all the tests fail if you keep track of offsets separately.
| @@ -441,9 +441,11 @@ func (i *Ingester) flushChunks(ctx context.Context, fp model.Fingerprint, labelP | |||
| ) | |||
|
|
|||
| // encodeChunk mutates the chunk so we must pass by reference | |||
| chunkMtx.Lock() | |||
There was a problem hiding this comment.
Same as toWireChunks. encodeChunk ultimately writes to c.blocks[*].offset in pkg/chunkenc/memchunk.go:674
| @@ -737,8 +737,9 @@ func (i *instance) getStats(ctx context.Context, req *logproto.IndexStatsRequest | |||
|
|
|||
| if err = i.forMatchingStreams(ctx, from, matchers, nil, func(s *stream) error { | |||
| // Consider streams which overlap our time range | |||
| s.chunkMtx.RLock() | |||
There was a problem hiding this comment.
We read s.chunk.Bounds() in shouldConsiderStreams
There was a problem hiding this comment.
but Bounds() already acquires the RLock here. do we still need this?
There was a problem hiding this comment.
Good spot - we probably don't need this change then. Grabbing the write lock when encoding should be enough since that is what causes the conflict.
benclive
changed the title
benclive
changed the title
| @@ -441,9 +441,11 @@ func (i *Ingester) flushChunks(ctx context.Context, fp model.Fingerprint, labelP | |||
| ) | |||
|
|
|||
| // encodeChunk mutates the chunk so we must pass by reference | |||
| chunkMtx.Lock() | |||
| if err := i.encodeChunk(ctx, &ch, c); err != nil { | |||
| return err | |||
There was a problem hiding this comment.
should you unlock here too ?
There was a problem hiding this comment.
Yep, on line 448 below. I can wrap this in a func if you prefer to use defer
What this PR does / why we need it:
-race.