Skip to content

winmm: synchronize completion notifications with waiting - #313

Open
kumagi wants to merge 1 commit into
ebitengine:mainfrom
kumagi:codex/winmm-callback-wakeup
Open

kumagi wants to merge 1 commit into
ebitengine:mainfrom
kumagi:codex/winmm-callback-wakeup

Conversation

@kumagi

@kumagi kumagi commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

What issue is this addressing?

None

What type of issue is this addressing?

bug

What this PR does | solves

A WOM_DONE notification can arrive after waitUntilHeaderAvailable checks the queued headers but before it calls Wait. Because the callback signals without acquiring the waiting mutex, the notification can be lost and playback can wait indefinitely. Hold the same mutex around Signal so notification and entering Wait are synchronized.

Validation: a private race harness using the extracted callback forced completion between the predicate check and Wait. The original callback lost the notification; the fixed callback woke the waiter. No waveOut calls are added to the callback. Native Windows playback was not tested.

Native macOS builds and internal package race tests passed, as did Windows 386/amd64/arm64 and WebAssembly cross-builds. The full macOS suite fails AudioQueueStart and then times out in TestSetBufferSize on both the unchanged upstream and this checkout; macOS vet reports the same pre-existing driver_darwin.go unsafe.Pointer warning.

@hajimehoshi hajimehoshi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The lost wakeup is real. Cond.Wait registers the waiter before unlocking, but a Signal with no registered waiter does nothing. The driver clears WHDR_INQUEUE outside cond.L, so nothing orders the signal against the check-then-Wait in waitUntilHeaderAvailable.

That said, I'd rather not take cond.L in the callback:

  • cond.L is held across mux.ReadFloat32s + waveOutWrite in appendBuffers, and across waveOutUnprepareHeader + waveOutClose in closeLoop. With this change, the winmm callback thread waits on a thread that is inside a wave function. The waveOutProc docs allow EnterCriticalSection, but say that calling other wave functions from the callback causes deadlock. This creates the same dependency indirectly.
  • 814d00b stopped queueing headers in the callback because it misbehaved when a headset was connected or disconnected. That is the case this change would need native Windows testing for.
  • The race has limited impact. The predicate is false only when all headers are queued, so a lost signal usually delays refilling by one header while the others keep playing. A permanent stall needs every queued header to complete inside that window.

headers, buf32 and waveOut are only touched by the loop goroutine, so cond.L doesn't protect any shared state now. Could you replace the sync.Cond with a channel with a buffer of 1? A pending send stays in the buffer, so no wakeup is lost, and the callback never blocks:

// in winmmContext
headerDone chan struct{} // make(chan struct{}, 1)

// in waveOutOpenCallback, after the WOM_DONE check
select {
case theWinMMContext.headerDone <- struct{}{}:
default:
}

func (c *winmmContext) waitUntilHeaderAvailable() bool {
	for !c.isHeaderAvailable() && c.err.Load() == nil {
		<-c.headerDone
	}
	return c.err.Load() == nil
}

appendBuffers and closeLoop can then drop the cond.L locking. loopEndCh is never set to a non-nil value, so it's left out above. Switching to CALLBACK_EVENT, as the TODO in start suggests, would also work, but it's a bigger change.


Drafted by Claude (Claude Code), on behalf of @hajimehoshi.

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