Fix histogram data race under -workers>1 + clean reporter shutdown - #119
Merged
Conversation
…116) hdrhistogram is not concurrency-safe, but the plain per-label/total/inst histograms were recorded from every worker goroutine (and read/reset by the periodic reporter) with no lock -- so percentiles were computed from racily mutated state and increments were lost (a race-instrumented binary reported 9-51 DATA RACEs at the default -workers 8). The op/byte counts were already exact via the atomic counters from #115; this fixes the percentile corruption. - Add histogramsMutex guarding all plain-histogram writes (worker record path) and the reporter's reads + inst Reset. The two histogram maps keep their own mutexes; the counters stay atomic. - Extract the per-command recording into recordCmdStat so the record path is unit-testable, and simplify the label switch. - Give the periodic reporter a clean shutdown (stopReport/reportDone + a select loop, ticker stopped): it now exits before the final read-out, so it no longer races GetTimeSeriesMap/summary over the histograms or the *Ts slices (fixes the leaked-reporter-goroutine timeseries race too). Reporter reads are snapshotted under the lock, keeping the log I/O out of the critical section. Tests: TestRecordCmdStatConcurrentIsRaceFree (fails under -race if the lock is removed -- verified) and TestReportStopsCleanly. integration-test now runs with -race so the benchmark_runner concurrency tests actually detect regressions. Validated E2E: race-instrumented binary, -workers 8, 0 DATA RACEs (was 9-51); TotalOps==dbsize exact; quantiles sane. Closes #116
…eport() concurrently Two fixes from the pre-merge adversarial review: - Bound the reporter-shutdown join. RunBenchmark previously did an unbounded `<-reportDone`. The reporter's only unbounded blocking point is its progress log write (outside histogramsMutex), so a wedged output consumer could stall it there and hang the whole run at the join. Wait with a 2s timeout instead; on timeout, take+release histogramsMutex as a happens-before barrier against a reporter improbably descheduled mid-critical-section (a reporter stalled in its log write holds no lock and does no further shared access once it unblocks). Normal runs are unaffected (the reporter's select returns immediately on stopReport). Note: this does NOT fully fix the pre-existing "hang on a stalled consumer -> no JSON" bug (summary() also logs before writing the result); that affects master too and is tracked separately. - Add TestReportConcurrentWithRecordIsRaceFree, which drives the REAL report() goroutine concurrently with recordCmdStat writers and reads after shutdown. The existing test only exercised a hand-rolled reader, so a report()-side lock regression would have slipped through; verified the new test fails with a DATA RACE under -race when report()'s histogram reads are unlocked. - Fix a stale comment (the benchmark_runner race guards run under `make integration-test`, not `make unit-test`).
Contributor
Author
|
15-way adversarial review (Opus 4.8) on the current HEAD — verdict: merge. Race-freedom empirically proven across a workers{1,4,8,16}×pipeline{1,4} + duration matrix (0 DATA RACEs; guard mutation-verified), extraction/shutdown/lock-scope/regression all clean, and a fair A/B shows no throughput regression (mutex is off the measured-latency path). The final reviewer caught one real issue: the unbounded |
|
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
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.
Closes #116.
Problem
hdrhistogramis not concurrency-safe, but the plain per-label / total /inst_*histograms were recorded from every worker goroutine (work()), and read +Reset()by the periodicreport()goroutine, with no lock. So:OverallQuantiles, the live median column) were computed from racily-mutated state, and increments were lost;DATA RACEs at the default--workers 8.(The op/byte counts were already made exact via the atomic counters in #115; the two histogram maps were already mutex-guarded. This PR closes the remaining plain-histogram race.)
There was also a leaked reporter goroutine:
report()looped ontime.NewTicker(period).Cwith no exit, so it kept touching the histograms and the*Tsslices pastwg.Wait()— racing the finalGetTimeSeriesMap/summaryread-out (a distinct slice append-vs-sort race).Fix
histogramsMutexguards all plain-histogram writes on the worker record path and the reporter's reads +instReset(). Counters stay atomic; the two maps keep their own mutexes.recordCmdStatso the per-command record path is unit-testable (and simplify the label switch).stopReport/reportDone+ aselectloop with the ticker stopped.RunBenchmarkstops the reporter afterwg.Wait()and before the final read-out, so it no longer runs concurrently withGetTimeSeriesMap/summary/GetOverallQuantiles— fixing the timeseries race too. Reporter values are snapshotted under the lock so the log I/O stays out of the critical section.Validation
--workers 8, 15k HSETs: 0DATA RACEs (was 9–51);TotalOps == dbsizeexact; quantiles sane (q50≈0.11ms, q99≈0.31ms).TestRecordCmdStatConcurrentIsRaceFree— 8 writers + a reporter-style reader; verified it fails withDATA RACEunder-racewhen the lock is removed, passes with it.TestReportStopsCleanly— the reporter exits within 2s ofclose(stopReport)(no goroutine leak).make integration-testnow runs with-raceso the benchmark_runner concurrency tests actually detect a regression. Full suite green under-race.Scope
Percentile/latency histograms only. The per-command error-count inflation in pipelines (#118) and the reply-capture latency cost (#117) remain separate.