mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.53k stars 1.28k forks source link

avoid data race on m_pStream #13899

Open m0dB opened 4 days ago

m0dB commented 4 days ago

This fixes several tsan warnings related with stopping / closing portaudio streams, including #13870.

Apart from access to a non-atomic variable m_pStream, the real problem here was that readProcess() writeProcess() would take a copy of m_pStream:

PaStream* pStream = m_pStream;

and close() would then call Pa_StopStream(m_pStream); Apparently stopping a stream and still accessing it in the callback leads to data races, at least on macOS.

Now, it the portaudio docs I read:

Pa_StopStream() is designed to make sure that the buffers you've processed in your callback are all played, which may cause some delay. Alternatively, you could call Pa_AbortStream(). On some platforms, aborting the stream is much faster and may cause some data processed by your callback not to be played. Another way to stop the stream is to return either paComplete, or paAbort from your callback. paComplete ensures that the last buffer is played whereas paAbort stops the stream as soon as possible. If you stop the stream using this technique, you will need to call Pa_StopStream() before starting the stream again.

So what I do: in close() I set an atomic variable to STOP, to indicate that readProcess and writeProcess should not access the stream anymore, and to indicate that the callback function should return paAbort, which will stop the stream. Pa_StopStream can then safely be called and m_pStream set to null.

As a side effect this also addresses a comment in the code about a failed intent to use Pa_AbortStream instead of PaStopStream.

This works well on macOS and I have not seen any portaudio related tsan warnings anymore. But since the platform specific aspects of Portaudio it is important to ensure this works as expected on Windows and Linux.

daschuer commented 3 days ago

m_pStream is meant to be an atomic. I think we can use the null state of this pointer instead of the additional atomic https://github.com/mixxxdj/mixxx/blob/4a35def01a54fd5a13b871a1c8e08ed8100f1d8e/src/soundio/sounddeviceportaudio.h#L62