Possible processor AtomicRefCell double borrow issue in VST3 and Clap

[emuell]

VST3 and Clap plugin instances may access their AudioThreadState's processor AtomicRefCell from different threads at the same time, the main thread and the audio thread. So it's very unlikely, but possible that this will randomly cause an already borrowed panic.

It's hard to replicate, but I can do so by creating a big bunch (~100) of plinth based plugins in Renoise, saving this as a song template. Loading such a song with many plinth instances will then sooner or later crash in IProcessorTrait::setProcessing:

plugin-things/plinth-plugin/src/formats/vst3/component.rs

Line 260 in 2002514

let mut processor = self.audio_thread_state.processor.borrow_mut();

In my reproducible case, the processor's mut_borrow is held by the main thread's setActivate function.

Before that, I got random crashes once a week or so with heavy usage while developing & testing my plugin.

---

The problem is conceptionally quite clear though:

VST3's and CLAP's active/deactivate functions may only be called from the main thread. Those (re)create the processor. The VST3 setprocessing and CLAP's reset functions on the other hand, may be called from the audio thread only.

So even if a host follows the specs very closely (which isn't the usual case), it's unlikely, yet possible, that both setactive and setprocessing may get called at the same time.

---

Only way I see to fix that would be using a Mutex here, but maybe you've got a better idea here.

As Mutex on Rust uses a Futex or something comparable lightweight to check if the mutex is locked, the uncontended access should be, in theory, roughly as fast as it is now with the AtomicRefCell.

Using try_borow_mut with the AtomicRefCell could work here too, but it's going to be hard to handle the can't access case.

It's definitely super ugly to use a lock in audio time here - that feels wrong - but likely is just fine in practice here as it basically never locks in practice. And when it locks, it does so while processors get rebuilt.

---

BTW: Just have seen you've released your plugin. Congrats! it looks fabulous. I hope all is going well and as planed.

[ilmai]

I recently moved Processor creation from setActive() to setupProcessing() because a user was reporting a reproducible crash in Digital Performer which does several function calls a bit... differently to other hosts. Does this fix the issue for you?

(and thanks, launch went well!)

[emuell]

Actually, I don't really have a problem. Just wanted to point out that this can, and thus will, cause trouble with this and that host right now and in future. Because it's a race condition, it always will be hard up to impossible to replicate, so I think it's worth nailing this down in theory.

We already had ensured in Renoise that process never is called during activate calls, but we didn't guard setProcessing. I've fixed that now, because this could be problematic with other plugins as well.

setProcessing just moves the problem and as mentioned above, also may happen in CLAP. I think AtomicRefCell simply is the wrong solution to Sync here. It satisfies the Rust compiler, but doesn't guarantee that it behaves correctly in runtime in all possible scenarios. After all, it will just ensure that the plugin cleanly crashes instead of corrupting memory here, which is nice, but really doesn't help users. All they see is that the plugin crashed.

[ilmai]

The issue with using Mutex here is that processor is accessed mutably in process() which is exactly the place where you don't want syscalls. Even if the underlying implementation is a futex, it's still a syscall which might have an unbounded execution time so it's a no-no. I'll see if there's something else I could do here.

[emuell]

There's no guarantee, but usually there is no syscall in userspace locks when there is no contention. And the Rust std Mutex is a userspace lock: it's not shared/locking across process boundaries.

Also, the only case where some locking, contention actually is happening in this case, is when the processor gets rebuilt from the main thread (setActivate or now setupProcessing). And this only happens on sample rate or bus layout or other other structural changes, so it's definitely not great, but IMHO practical to lock instead of crashing here.

Just checked how the Rust Mutex is implemented across platforms:
Windows: SRW Lock
Linux: Futex
MacOS: os_unfair_lock

SRW Lock and Futex will both do an atomic load with a barrier, just like AtomicRefCell does in the fast path without contention. In os_unfair_lock_lock it seems a bit more complicated and needs to be checked.

That said, atomic operations with barriers are not strictly RT-safe either. Latency here is small but also non-deterministic due to the cache syncs across CPUs, but usually fast enough for audio and thus RT-safe enough.

Using a spin lock instead of a Mutex may sound like an alternative, but I don't think that a hand rolled spin can be really faster here than the battle proofed Mutex.

I'd simply try out a mutex and profile if there really is a noticeable difference to AtomicRefCell.

I'll check some possible other alternatives as well in depth. Wanted to do that for some other project anyway.

[emuell]

Parking Lot's mutex could be a good cross-platform alternative. In the fast path, this is guaranteed to be just an atomic load and compare exchange on all platforms: https://docs.rs/parking_lot/latest/src/parking_lot/raw_mutex.rs.html#66

And when using try_lock in process, there actually never would be a syscall or wait at all. When try_lock on the mutex fails here, this means the main/GUI thread currently holds the lock and rebuilds the processor, so it's also fine to just return an empty buffer here instead of locking (where it now crashes in borrow_mut).

Something like this:

unsafe fn process(&self, data: *mut ProcessData) -> tresult {
    let data = unsafe { &mut *data };

    let parameter_change_iterator = ParameterChangeIterator::new(data.inputParameterChanges, *self.pitch_bend_parameter_ids.borrow());
    let event_iterator = EventIterator::new(data.inputEvents);
    let all_events = event_iterator.chain(parameter_change_iterator);

    // Locking only will fail if setupProcessing/setProcessing is concurrently running. 
    // Skip the buffer rather than blocking or causing a panic.
    let Some(mut processor_guard) = self.audio_thread_state.processor.try_lock() else {
        // skip buffer gracefully and TODO: clear audio buffers
        return kResultOk; 
    };

    let Some(processor) = processor_guard.as_mut() else {
        return kResultFalse; // not yet set up - error
    };

    // [... old processing stuff]
}

[ilmai]

Parking Lot's mutex could be a good cross-platform alternative. In the fast path, this is guaranteed to be just an atomic load and compare exchange on all platforms: https://docs.rs/parking_lot/latest/src/parking_lot/raw_mutex.rs.html#66

Good call! I'll look into replacing AtomicRefCell with this.

[ilmai]

This is now fixed in 20fbd37

Could you do a review of the change?