Properly close microphone before malgo chunk channel

[edaniels]

Prior to this, there's a chance you would close the chunk channel while malgo is trying to send on it. Instead, it's better to stop the malgo device and then close. That also means we need to discard chunks when trying to stop due to the buffered channel.

[at-wat]

Is <-m.chunkChan always received after <-closeDone?
I guess m.chunkChan and closeDone may be triggered almost at the same time and may return before consuming m.chunkChan at low chance.

func (m *microphone) Close() error {
	if m.deviceCloseFunc != nil {
		m.deviceCloseFunc()
	}
	// Don't close m.chunkChan to avoid panic due to `send to closed chan`
	// Go GC collects unclosed chan with buffered value when unused:
	//   https://groups.google.com/g/golang-nuts/c/KtIyc5lTRJY/m/6PK6aK5QKHIJ
	return nil
}
...
func (m *microphone) AudioRecord(inputProp prop.Media) (audio.Reader, error) {
	...
	ctx, cancel := context.WithCancel(context.Background())
	onRecvChunk := func(_, chunk []byte, framecount uint32) {
		select {
		case m.chunkChan <- chunk:
		case <-ctx.Done():
		}
	}
	...
	m.deviceCloseFunc = func() {
		closeDeviceOnce.Do(func() {
			cancel() // Unblock onRecvChunk
			device.Stop()
			device.Uninit()
		})
	}

would be more safe?

[edaniels]

Yup, good catch. Will use your version. The whole point is to unblock anyway, not consume.

[edaniels]

Also check your gophers slack!

[edaniels]

I'll still use your way but the original way is also safe because closeDone only happens after we stop the device which means the callback will never fire and no one will get stuck.

[edaniels]

@at-wat updated. I left the intentional close in to not rely on an GC implementation detail

[codecov]

Codecov Report

Merging #436 (dcaf53c) into master (5215057) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #436   +/-   ##
=======================================
  Coverage   47.32%   47.32%           
=======================================
  Files          67       67           
  Lines        4463     4463           
=======================================
  Hits         2112     2112           
  Misses       2226     2226           
  Partials      125      125           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[at-wat]

I think this can be called at exactly same timing with m.chunkChan <- chunk and cause panic by low chance.
(same as #414 (comment))

[edaniels]

I don't think it can be. The close function calls Stop/Uninit (turns out only Uninit is required). Both of those functions wait for the worker thread to return and ultimately will no longer call the stop/data callbacks. Therefore there are no more sends on the channel after and we can safely close it. Additionally you can see the Remarks section in https://miniaudio.docsforge.com/master/api/#ma_device_callback_proc referring to deadlocks and https://miniaudio.docsforge.com/master/api/ma_device_uninit/ showing the worker thread being waited on to terminate.

[at-wat]

Oh I see, as long as the uninit waits for ongoing callback return, it should be safe.

Maybe moving close(m.chunkChan) into deviceCloseFunc would be better at the readability about this safety logic.

[edaniels]

moved!

[at-wat]

As it's in Once,

Suggested change

	if m.chunkChan != nil {
	close(m.chunkChan)
	m.chunkChan = nil
	}
	close(m.chunkChan)
	m.chunkChan = nil

[edaniels]

I left that there because we don't check if we've called Open, so the channel may be nil, hypothetically

[at-wat]

LGTM!