Possible race condition in AudioProcessor::isSuspended()?

Audio Plugins
1

We’re having some issues that have arisen with our calls to AudioProcessor::suspendProcessing() and AudioProcessor::isSuspended() on Apple Silicon M1 machines. We use RAII to have a small “ScopedSuspended” object to keep track of the calls so fairly sure there are the correct number of matching calls to suspend and resume. But I notice that isSuspended() doesn’t take the lock when accessing the state. Should it be changed to this?:

    /** Returns true if processing is currently suspended.
        @see suspendProcessing
    */
    bool isSuspended() noexcept { const ScopedLock sl (callbackLock); return suspended; }
2

suspended is a boolean, I am pretty sure the operation would always be atomic, so the lock wouldn’t make a difference. This would be an issue if you wanted to keep other code from changing the state of suspended the entire time of set of operations, but for just retrieving the value there will be no difference.

3

Yeah thanks, that’s what I was thinking. But in the context below are we sure that the reads and writes are definitely not going to be reordered by the compiler or CPU? On all CPUs, especially ARM?

    class ScopedSuspendProcessing
    {
    public:
        ScopedSuspendProcessing (AudioProcessor& processor)
        :   p (processor), wasSuspended (processor.isSuspended())
        {
            p.suspendProcessing (true);
        }
        
        ~ScopedSuspendProcessing()
        {
            p.suspendProcessing (wasSuspended);
        }
        
    private:
        AudioProcessor& p;
        const bool wasSuspended;
    };

We have a number of functions where we suspend processing, and not all subsets of those functions are called in each case. So we use something like that to suspend and restore the previous state using RAII. I’m sure @timur will enjoy this one! :grin:

4

Ah. I would say this is a specialized use case. ie. normal usage does not require the overhead of taking the lock to get the value. If any change were made, I would vote for an additional API which has the lock.

5

If you write a data race it’s undefined behaviour which means you can’t rely on it to “work” forever. It could be a compiler update or new optimisation that breaks the behaviour in a subtle way.

6

I think the clean solution is if the processor has an atomic counter, which counts how much nested suspends are done, even with a lock inside “isSuspended” you are note safe, because there is gap between initialisation of wasSuspended and p.suspendProcessing(true)

something like this

       void beginSuspend ()
        {
            ScopedLock sl (suspendCS);
            if (nestedSuspends == 0)
            {
                suspendProcessing (true);
            }
            ++nestedSuspends;
        };

        void endSuspend ()
        {
            ScopedLock sl (suspendCS);
             --nestedSuspends;
            if (nestedSuspends == 0)
            {
                suspendProcessing (false);
            }
        };

and ScopedSuspendProcessing should these two methods beginSuspend/endSuspend

1 Like