diff --git a/fix.txt b/fix.txt index 1531484..64fbb04 100644 --- a/fix.txt +++ b/fix.txt @@ -85,7 +85,7 @@ Priority 1: transport, lifecycle, and correctness a cancellation context plus WaitGroup/done channel; serialize Start, Stop, reopen, and Destroy; wait before CaptureCloseDevice. -10. Renderer can be used after it is closed +[x] 10. Renderer can be used after it is closed Files: gumble/gumbleopenal/stream.go, gumble/gumble/audiolisteners.go Existing OnAudioStream goroutines can run cleanup after Destroy closes renderCh. Their final render call then panics sending on a closed channel. diff --git a/gumble/gumbleopenal/stream.go b/gumble/gumbleopenal/stream.go index ae6cac9..884040f 100644 --- a/gumble/gumbleopenal/stream.go +++ b/gumble/gumbleopenal/stream.go @@ -61,10 +61,12 @@ type Stream struct { micVolume atomic.Uint32 // float32 stored as bits sourceStop chan bool - deviceSink *openal.Device - contextSink *openal.Context - renderCh chan renderCommand - renderDone chan struct{} + deviceSink *openal.Device + contextSink *openal.Context + renderMu sync.RWMutex + renderCh chan renderCommand + renderDone chan struct{} + renderClosed bool noiseProcessor NoiseProcessor noiseProcessorRight NoiseProcessor @@ -159,8 +161,11 @@ func New(client *gumble.Client, inputDevice *string, outputDevice *string, test } func (s *Stream) startRenderer() { + s.renderMu.Lock() + s.renderClosed = false s.renderCh = make(chan renderCommand) s.renderDone = make(chan struct{}) + s.renderMu.Unlock() ready := make(chan struct{}) go func() { runtime.LockOSThread() @@ -177,11 +182,18 @@ func (s *Stream) startRenderer() { <-ready } -// render executes fn on the sole OS thread that owns the OpenAL context. -func (s *Stream) render(fn func()) { +// render executes fn on the sole OS thread that owns the OpenAL context. It +// returns false after shutdown instead of sending to a closed renderer channel. +func (s *Stream) render(fn func()) bool { + s.renderMu.RLock() + defer s.renderMu.RUnlock() + if s.renderClosed || s.renderCh == nil { + return false + } command := renderCommand{fn: fn, done: make(chan struct{})} s.renderCh <- command <-command.done + return true } func (s *Stream) AttachStream(client *gumble.Client) { @@ -296,7 +308,12 @@ func (s *Stream) Destroy() { openal.NullContext.Activate() s.contextSink.Destroy() }) - close(s.renderCh) + s.renderMu.Lock() + if !s.renderClosed { + close(s.renderCh) + s.renderClosed = true + } + s.renderMu.Unlock() <-s.renderDone s.contextSink = nil } diff --git a/gumble/gumbleopenal/stream_regression_test.go b/gumble/gumbleopenal/stream_regression_test.go new file mode 100644 index 0000000..2c353b6 --- /dev/null +++ b/gumble/gumbleopenal/stream_regression_test.go @@ -0,0 +1,16 @@ +package gumbleopenal + +import "testing" + +// Regression: audio cleanup could send a final render command after Destroy +// had closed renderCh, panicking instead of safely discarding that work. +func TestRenderRejectsWorkAfterShutdown(t *testing.T) { + s := &Stream{renderClosed: true} + called := false + if s.render(func() { called = true }) { + t.Fatal("closed renderer accepted work") + } + if called { + t.Fatal("closed renderer executed work") + } +}