Skip to content

progress: hold the lock while checking timeStart in Tracker.Start - #437

Open
troclaux wants to merge 1 commit into
jedib0t:mainfrom
troclaux:fix/tracker-start-race
Open

troclaux wants to merge 1 commit into
jedib0t:mainfrom
troclaux:fix/tracker-start-race

Conversation

@troclaux

Copy link
Copy Markdown

Proposed Changes

  • Tracker.Start() read t.timeStart without holding t.mutex, while startWithoutLock() (reached via Increment, IncrementWithError, SetValue or another Start()) writes it under the lock. Calling Start() while another goroutine feeds progress is a data race; two concurrent Start() calls could also both see a zero timeStart and start twice.
  • Start() now takes the write lock, checks timeStart.IsZero() and calls startWithoutLock() in one critical section, the same pattern IsStarted() and incrementWithoutLock() use.
  • Added TestTracker_Start_Concurrent: two goroutines call Start() while two call Increment(1) on the same tracker. Before the fix, go test -race -run TestTracker_Start_Concurrent ./progress/ fails with WARNING: DATA RACE (tracker.go:202 read vs. tracker.go:261 write). After the fix it passes over 20 runs.

Testing: make test (fmt, vet, golangci-lint, gocyclo, tests with 100% coverage) and make test-race both pass.

@sonarqubecloud

Copy link
Copy Markdown

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 39819707428

Coverage remained the same at 100.0%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 3 of 3 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 5166
Covered Lines: 5166
Line Coverage: 100.0%
Coverage Strength: 1.21 hits per line

💛 - Coveralls

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.

2 participants