Fix send-under-lock deadlock in MockTimer and MockTicker - #10
Merged
Conversation
Both MockTimer and MockTicker delivered their tick by sending on an unbuffered channel while holding the internal mutex. A consumer that calls Stop() (or Reset(), or drains the channel) needs that same mutex, so if a tick was mid-send when the consumer reached Stop(), Stop() blocked on the lock, the send never completed, and the lock was never released -- a permanent deadlock. This is the deadlock that #9 reverted #6-#8 to avoid. Fix: deliver the tick with the lock released. - MockTimer/MockTicker now snapshot the value to send under the lock, drop the lock around the channel send (or AfterFunc), then re-acquire it, so a concurrent Stop/Reset/drain can always make progress. - MockTimer keeps a single long-lived process goroutine re-armed via the cond variable instead of spawning a fresh goroutine on every Reset (the old Reset leaked a goroutine and raced multiple deliverers on one channel). - MockTicker advances its deadline relative to the now value captured at fire time, not the live now, so an Advance during the (now lock-free) delivery window no longer skips a tick. - register() now takes the mutex itself (fixing the pre-existing data race on the subscriber list); callers already holding the lock use registerLocked. MockClock.After / MockTimer.Reset use registerLocked, and MockClock.NewTicker releases the lock before constructing the ticker to avoid re-entrant locking. Delivered values are snapshotted under the lock, resolving the -race failure where the released-lock send read now concurrently with Advance. Adds stress regression tests for both types (concurrent Stop during fire) that hang and fail via the test timeout if the deadlock regresses. Verified with `go test -race -count=10` on the full suite.
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
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.
Problem
Both
MockTimerandMockTickerdelivered a tick by sending on an unbuffered channel while holding the internal mutex. Any consumer that callsStop(),Reset(), or drains the channel needs that same mutex — so if a tick was mid-send when the consumer reachedStop(),Stop()blocked on the lock, the send never completed, and the lock was never released. Permanent deadlock.This is the deadlock that #9 reverted #6–#8 to avoid ("Revert recent changes until we figure out deadlocks"). This PR is the "figure it out" follow-up.
It shows up in real usage as a hung test: a heartbeat-style consumer doing
Stop()→ drain →Reset()on each data chunk while a background loop advances the mock clock past the deadline. Observed as apanic: test timed outwith goroutines parked on the mock'sLock()while one goroutine is blocked on the channel send holding it.Fix
Deliver the tick with the lock released.
MockTimer/MockTickersnapshot the value to send under the lock, drop the lock around the channel send (orAfterFunc), then re-acquire — so a concurrentStop/Reset/drain can always make progress.MockTimernow keeps a single long-livedprocessgoroutine, re-armed via the cond variable, instead of spawning a fresh goroutine on everyReset(the oldResetleaked a goroutine and raced multiple deliverers on one channel).MockTickeradvances its deadline relative to thenowcaptured at fire time, not the livenow, so anAdvanceduring the (now lock-free) delivery window no longer skips a tick (preserves slow-reader tick-dropping semantics).register()now takes the mutex itself, fixing the pre-existing data race on the subscriber list; lock-holding callers use a newregisterLocked.MockClock.After/MockTimer.ResetuseregisterLocked, andMockClock.NewTickerreleases the lock before constructing the ticker to avoid re-entrant locking on the non-reentrant mutex.Delivered values are snapshotted under the lock, which also resolves a
-racefailure where the released-lock send readnowconcurrently withAdvance.Tests
Adds stress regression tests for both types (
TestMockTimerConcurrentStopDuringFire,TestMockTickerConcurrentStopDuringFire) that reproduce the concurrent-Stop-during-fire deadlock; they hang and fail via the test timeout if it regresses.Verified:
go test -race -count=10 ./passes on the full suite (the new tests deadlocked 100% of the time before this change).