Skip to content

Commit 022c317

Browse files
committed
Fixed Histogram Concurrency Issue
Issue: In Histogram, some values were being dropped when the histogram is accessed concurrently by multiple threads. Fix: In getOrCreateHistogram, the code was copying the dictionary into a local variable, and checking it for the label without a lock. If not there, it would take the lock again to add it. But in line 224, after taking the lock again, it was rechecking the local variable, which of course would not have changed in the meantime. But the original dictionary could have changed, and that's the one we should check the second time. Improved the concurrency test, as it was missing the bug by only checking the total count and sum without labels.
1 parent 488b935 commit 022c317

File tree

2 files changed

+13
-4
lines changed

2 files changed

+13
-4
lines changed

Sources/Prometheus/MetricTypes/Histogram.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ public class PromHistogram<NumType: DoubleRepresentable>: PromMetric {
221221
return histogram
222222
} else {
223223
return lock.withLock {
224-
if let histogram = subHistograms[labels] {
224+
if let histogram = self.subHistograms[labels] {
225225
precondition(histogram.name == self.name,
226226
"""
227227
Somehow got 2 subHistograms with the same data type / labels

Tests/SwiftPrometheusTests/HistogramTests.swift

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ final class HistogramTests: XCTestCase {
2929
helpText: "Histogram for testing",
3030
buckets: Buckets.exponential(start: 1, factor: 2, count: 63))
3131
let elg = MultiThreadedEventLoopGroup(numberOfThreads: 8)
32-
let semaphore = DispatchSemaphore(value: 2)
32+
let semaphore = DispatchSemaphore(value: 0)
3333
_ = elg.next().submit {
3434
for _ in 1...1_000 {
3535
let labels = DimensionLabels([("myValue", "1")])
@@ -51,9 +51,18 @@ final class HistogramTests: XCTestCase {
5151
semaphore.signal()
5252
}
5353
semaphore.wait()
54+
semaphore.wait()
5455
try elg.syncShutdownGracefully()
55-
XCTAssertTrue(histogram.collect().contains("my_histogram_count 4000.0"))
56-
XCTAssertTrue(histogram.collect().contains("my_histogram_sum 4000.0"))
56+
57+
let output = histogram.collect()
58+
XCTAssertTrue(output.contains("my_histogram_count 4000.0"))
59+
XCTAssertTrue(output.contains("my_histogram_sum 4000.0"))
60+
61+
XCTAssertTrue(output.contains(#"my_histogram_count{myValue="1"} 2000.0"#))
62+
XCTAssertTrue(output.contains(#"my_histogram_sum{myValue="1"} 2000.0"#))
63+
64+
XCTAssertTrue(output.contains(#"my_histogram_count{myValue="2"} 2000.0"#))
65+
XCTAssertTrue(output.contains(#"my_histogram_sum{myValue="2"} 2000.0"#))
5766
}
5867

5968
func testHistogramSwiftMetrics() {

0 commit comments

Comments
 (0)