Skip to content

reporter: fix race condition in FrameKnown#471

Merged
christos68k merged 1 commit intoopen-telemetry:mainfrom
zystem-io:th0rex/fix-race
May 28, 2025
Merged

reporter: fix race condition in FrameKnown#471
christos68k merged 1 commit intoopen-telemetry:mainfrom
zystem-io:th0rex/fix-race

Conversation

@th0rex
Copy link
Copy Markdown
Contributor

@th0rex th0rex commented May 28, 2025

4377c74 switched the inner value of pdata.Frames to be a LRU, instead of a map. It uses GetAndRefresh in FrameKnown to update the lifetime of an element, if it exists.

GetAndRefresh is a write operation, but the lock that is taken around this operation was not updated in the commit, thus we still only hold a read lock here, leading to potential data races.

This commit changes the code to take a write lock instead, so only one Goroutine can refresh the key at a time.

4377c74 switched the inner value of
`pdata.Frames` to be a LRU, instead of a map. It uses `GetAndRefresh` in
`FrameKnown` to update the lifetime of an element, if it exists.

`GetAndRefresh` is a write operation, but the lock that is taken around
this operation was not updated in the commit, thus we still only hold a
read lock here, leading to potential data races.

This commit changes the code to take a write lock instead, so only one
Goroutine can refresh the key at a time.
@th0rex th0rex requested review from a team as code owners May 28, 2025 15:12
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 28, 2025

CLA Signed


The committers listed above are authorized under a signed CLA.

Copy link
Copy Markdown
Member

@florianl florianl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 👍

@christos68k christos68k merged commit 13d4300 into open-telemetry:main May 28, 2025
25 checks passed
@athre0z athre0z deleted the th0rex/fix-race branch May 28, 2025 15:40
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.

4 participants