Repository navigation
Conversation
hajimehoshi
left a comment
There was a problem hiding this comment.
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.Lis held acrossmux.ReadFloat32s+waveOutWriteinappendBuffers, and acrosswaveOutUnprepareHeader+waveOutCloseincloseLoop. With this change, the winmm callback thread waits on a thread that is inside a wave function. The waveOutProc docs allowEnterCriticalSection, 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.
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.