Fix race condition in PrometheusSink during concurrent metric updates #191
+171
−136
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test demonstrates a race condition that occurs when PrometheusSink
is used from multiple goroutines concurrently, resulting in missed
counter updates.
The test spawns multiple goroutines that increment the same counter
simultaneously and verifies that the final value matches the expected
total.
Example:
go test -v ./prometheus -run TestPrometheusRaceCondition$ -count=1
=== RUN TestPrometheusRaceCondition
prometheus_race_test.go:63: Race condition detected: got 152 want 200
--- FAIL: TestPrometheusRaceCondition (0.00s)
FAIL
FAIL github.com/hashicorp/go-metrics/prometheus 0.253s
FAIL
Fixes a race condition in PrometheusSink where concurrent updates to the same metric from multiple goroutines could result in missed updates or incorrect values. The previous copy-on-write pattern was racy because multiple goroutines could read the old value, create separate copies with their updates, and overwrite each other when storing back to the sync.Map. This change adds a sync.RWMutex to each gauge, summary, and counter struct to protect concurrent access to the underlying Prometheus metric and the updatedAt timestamp. The fix also uses LoadOrStore instead of Load+Store to properly handle creation races when multiple goroutines attempt to create the same metric simultaneously.
|
|
Also updated the prometheus_race_test.go to be in line with other tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
PrometheusSinkhad a race condition when multiple goroutines concurrently created the same metric for the first time. Each goroutine would create its own separate Prometheus counter/gauge/summary instance, increment it, and then store it using Store(). The last write would win, but all other metric instances with their accumulated values would be lost.This fix uses
LoadOrStore()instead ofStore()to ensure only one metric instance exists per key, then updates that single instance regardless of which goroutine won the creation race.Related Issue
N/A (discovered during testing/usage)
How Has This Been Tested?
TestPrometheusRaceConditionwhich spawns multiple goroutines that concurrently increment the same counter and verifies the final value matches the expected totalBefore the fix:
With this fix applied the test always passes