Skip to content

fix(router): swap requestsMetric slice under exclusive Lock instead of RLock in collectMetrics - #7266

Open
harsh4vardhan wants to merge 1 commit into
rudderlabs:masterfrom
harsh4vardhan:fix/router-collect-metrics-lock
Open

fix(router): swap requestsMetric slice under exclusive Lock instead of RLock in collectMetrics#7266
harsh4vardhan wants to merge 1 commit into
rudderlabs:masterfrom
harsh4vardhan:fix/router-collect-metrics-lock

Conversation

@harsh4vardhan

Copy link
Copy Markdown

Summary

Resolves #7261.

collectMetrics acquires requestsMetricLock.RLock() (shared) and
then sets rt.telemetry.requestsMetric = nil while holding it. Writing
shared state under RLock violates sync.RWMutex's contract.
trackRequestMetrics, called from every worker, takes the exclusive
Lock() to append to the same slice — a concurrent read+reset under
RLock races with those appends.

Fix

Take a short exclusive lock, swap the slice into a local snapshot,
release the lock, then iterate the snapshot outside the lock:

rt.telemetry.requestsMetricLock.Lock()
snapshot := rt.telemetry.requestsMetric
rt.telemetry.requestsMetric = nil
rt.telemetry.requestsMetricLock.Unlock()

for _, reqMetric := range snapshot { ... }

This is shorter under lock than the original (lock held only for the
pointer swap, not for the full iteration and diagnostics call) and
correct by sync.RWMutex contract.

harsh4vardhan

collectMetrics acquires RLock (shared) then writes to requestsMetric
(sets it to nil). Writing shared state under RLock violates sync.RWMutex
contract: RLock permits concurrent readers; a writer that modifies data
while holding RLock races with trackRequestMetrics callers that hold the
exclusive Lock.

Fix: take a short exclusive Lock, swap the slice into a local snapshot,
release the lock, then iterate the snapshot. This mirrors the pattern
used by trackRequestMetrics on the same field and keeps the lock held
for only the swap, not the full iteration + diagnostics call.

Fixes rudderlabs#7261

Signed-off-by: harsh4vardhan <hvardhan609@gmail.com>
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.

router: collectMetrics writes requestsMetric slice under RLock instead of Lock - data race under concurrent callers

1 participant