Skip to content

Fix send-under-lock deadlock in MockTimer and MockTicker - #10

Merged
efritz merged 1 commit into
mainfrom
fix/mock-timer-ticker-send-under-lock-deadlock
Jul 24, 2026
Merged

Fix send-under-lock deadlock in MockTimer and MockTicker#10
efritz merged 1 commit into
mainfrom
fix/mock-timer-ticker-send-under-lock-deadlock

Conversation

@efritz

@efritz efritz commented Jul 24, 2026

Copy link
Copy Markdown
Member

Problem

Both MockTimer and MockTicker delivered a tick by sending on an unbuffered channel while holding the internal mutex. Any consumer that calls Stop(), 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. 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 a panic: test timed out with goroutines parked on the mock's Lock() while one goroutine is blocked on the channel send holding it.

Fix

Deliver the tick with the lock released.

  • MockTimer/MockTicker snapshot the value to send under the lock, drop the lock around the channel send (or AfterFunc), then re-acquire — so a concurrent Stop/Reset/drain can always make progress.
  • MockTimer now 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 captured at fire time, not the live now, so an Advance during 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 new registerLocked. MockClock.After/MockTimer.Reset use registerLocked, and MockClock.NewTicker releases 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 -race failure where the released-lock send read now concurrently with Advance.

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).

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.
@efritz
efritz merged commit 89f8a8b into main Jul 24, 2026
1 check passed
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.

1 participant