The plugin API is unsound due to multi-threading

[askeksa]

Safe buffer creation in the host was just the warm-up. Now the real fun begins! 😉

TL;DR: To adhere to the requirements of Safe Rust, the Plugin API of the rust-vst crate needs to be restructured in a way that will require substantial changes to all existing plugins.

The problem

The VST API is multi-threaded. A host may call multiple methods concurrently on a plugin instance.

The way the rust-vst crate is structured, all methods have access to the same data - an instance of a type implementing the Plugin trait. In the presence of concurrency, this sharing causes data races, which is undefined behavior and violates the assumptions of Safe Rust. In practice, this leads to crashes and other weird behavior, as the Rust compiler assumes that access through a mutable reference is exclusive and performs optimizations and other transformations based on this assumption.

If the rust-vst crate is to be a safe wrapper around the VST API, it must be restructured in such a way that data races cannot occur. In Rust terms, this means that:

• If two mehods can be called concurrently, they cannot have mutable access to the same data.
• If data is (immutably) shared between methods that can be called concurrently, it must be Sync.

On the host side, we have the opposite problem: a host would potentially like to call plugin methods from multiple threads (as outlined in the next section), but it is currently not possible to do so in Safe Rust, because many of the methods require an exclusive reference to the plugin instance.

VST concurrency

The concurrency between VST plugin methods is, fortunately, not arbitrary. The multi-threading characteristics of VST plugins are described here. I found this description to be somewhat vague and incomplete, so in order to uncover the details of how a host might call into a plugin, I wrote a test plugin (which detects and reports which methods are called concurrently) and stress-tested it in Renoise (ran a few notes in a loop with parameter automation while tweaking everything in the GUI I could think of).

My understanding based on both of these sources is the following:

• Methods in a plugin are called from two threads: the GUI thread and the processing thread. There can be more than one processing thread, but calls will only come from one of them at a time, so for the purposes of concurrency, we can assume there is just one processing thread.

• The VST plugin methods fall into four categories:

  1. The setup methods (can_do, get_info, get_input_info, get_output_info, init, resume, suspend, set_block_size, set_sample_size and (presumably; not seen) get_tail_size). These methods are never called concurrently with anything. Furthermore, all of these methods (except suspend) are only ever called when the plugin is in the suspended state. All other methods are only called when the plugin is in the resumed state.

  2. The processing methods (process, process_f64 and process_events) are always called from the processing thread. Thus, they are never called concurrently with each other, but can be called concurrently with other methods (except the setup methods).

  3. The automation methods (set_parameter, change_preset) can be called either from the processing thread (for automation) or from the GUI thread (when parameters are manually changed in the host GUI). Thus, these can be called concurrently with themselves and each other, and with other methods (except the setup methods).

  4. The remaining methods (mostly parameter queries, preset handling and editor interaction) are always called from the GUI thread. Thus they are never called concurrently with each other, but can be called concurrently with the processing and automation methods.

Requirements

A solution to this issue should ideally fulfill the following requirements:

• A plugin written in Safe Rust never encounters data races (or other undefined behavior) when run in a host that follows the VST concurrency rules described in the previous section.

• A host written in Safe Rust is able to call plugin methods from multiple threads, subject to the VST concurrency rules. It is a bonus if the rules are enforced by the API (statically, dynamically, or some combination thereof) such that the host is not able to violate them.

• Calling a Rust plugin directly from a Rust host without going through the VST API is still possible and is still safe (i.e. safety should not depend on the VST API bridging code).

• The API is not too opinionated about how the plugin implements communication between the threads. In particular, it should be possible, within the API constraints, for the processing thread to be completely free of allocation and blocking synchronization.

Implementation

To achieve safety, the plugin state needs to be split into separate chunks of state such that methods that can be called concurrently do not have mutable access to the same chunk.

Note that since the automation methods can be called concurrently with themselves, this implies that these methods can't have mutable access to anything. All mutation performed by these methods must thus take place via thread-safe internal mutability (i.e. Mutex, RwLock, spinlocks, atomics and the like).

One way to split the state could be something like this:

// Exclusive to the processing thread.
trait PluginProcessing {
    fn process(&mut self, buffer: &mut AudioBuffer<f32>);
    fn process_f64(&mut self, buffer: &mut AudioBuffer<f64>);
    fn process_events(&mut self, events: &Events);
}

// Shared between threads and the main vessel for communication
// between the threads. This communication happens through
// thread-safe interior mutability.
// Note that all references to self are immutable.
trait PluginAutomation {
    fn set_parameter(&self, index: i32, value: f32);
    fn change_preset(&self, preset: i32);

    // The other parameter/preset methods can be placed here.
    // This will force these other methods to also work though
    // interior mutability, but it will reduce the amount of
    // communication necessary between separate state chunks.
}

// Main plugin trait.
trait Plugin {
    type Processing: PluginProcessing;
    type Automation: PluginAutomation + Sync;

    // Get a shared handle to the automation state.
    fn get_automation_handle(&mut self) -> Arc<Self::Automation>;

    // When a plugin is resumed, it relinquishes its access to the
    // processing state so that it can be passed to the processing
    // thread for exclusive access.
    fn resume(&mut self) -> Box<Self::Processing>;

    // To suspend a plugin, the host must pass the processing state
    // back in to prove that no other thread is accessing it.
    fn suspend(&mut self, Box<Self::Processing>);

    // Setup and remaining methods
}

This design achieves thread safety in plugins, and it prevents hosts from calling processing methods while the plugin is suspended. It does have a few drawbacks, though:

• It does not prevent the host from calling setup methods while the plugin is resumed. Such a restriction could be implemented by giving the host a "setup token" that it needs to pass to the setup methods (or maybe the methods are on the token itself). This token would be consumed by the resume method and given back by the suspend method. This could become somewhat unwieldy for both the plugin and the host, however.

• Using the standard Arc and Box types to control access to the Automation and Processing state chunks means that these chunks must be allocated on the heap. This could be avoided by introducing specialized wrapper types with similar semantics but which will allow the chunks to be embedded into the main plugin struct. But again, this would make the API more cumbersome to use.

• Adding associated types to the Plugin trait means the trait is no longer object safe, i.e. it can't be used for trait objects. This can be a problem for the pure Rust use case (bypassing the bridge).

• Some of the new methods can't have sensible default implementations unless we require the Processing and Automation types to implement Default. Such a requirement can be inconvenient, as the Processing chunk would usually want to contain a (non-optional) reference to the Automation chunk.

What to do now

Discuss. 😀

Then, we should make some prototypes to see how these ideas (and others we come up with) work in practice.

This change is a substantial undertaking (not least in fixing all existing plugins), but I think it is necessary before we can call our crate complete and stable. And if we manage to pull this off in a good way, it could make for a quite good case story (about wrapping an unsafe API safely) for This Week in Rust. 😌

[Boscop]

Thanks for investigating this..
I think if we can make the vst crate threadsafe without making it really unergonomic, we should strive towards doing that..
We could test this behavior also in other hosts, to see how they behave.
Btw, AFAIK, if the associated type is specified, it's possible to create a trait object if the trait is otherwise object safe..

[askeksa]

I think if we can make the vst crate threadsafe without making it really unergonomic, we should strive towards doing that..

Yes, ergonomics are very important. I think splitting up the methods as I have suggested is within reason. The most cumbersome part of it will be the communication via thread-safe interior mutability, but that is necessary in any case for safety. It will probably be most ergonomic to include all of the state/preset methods into the PluginAutomation trait (which then maybe should get a different name, since it becomes about more than automation).

We could test this behavior also in other hosts, to see how they behave.

Yes. That one should have been on my "what to do next" list. If other hosts completely violate the rules as formulated here, we need to rethink.

Btw, AFAIK, if the associated type is specified, it's possible to create a trait object if the trait is otherwise object safe..

It is, but since the associated types will likely be different for each different plugin, the trait objects will be specific to a particular plugin, which defeats their purpose.

[Boscop]

The assoc types could be boxed, like when a trait with async methods is turned into a trait object: https://boats.gitlab.io/blog/post/async-methods-ii/
But it costs another virtual call :/

[askeksa]

The cost of a virtual call will likely be insignificant for functions like these, which are called a few hundred times per second at most. And for the most common case of the functions being called by the bridge code in a particular plugin, the call targets will always be the same, so the branches will be well predicted, making the virtual calls basically as cheap as static calls. It does make the handles fat pointers, but that memory overhead is likely insignificant as well.

Using trait objects here will also solve the issue about default implementations, as we can just have dummy implementations of the traits which are returned by default.

[askeksa]

On closer inspection, the trait object route does not play well together with passing the processing object out and in through resume/suspend as suggested. The setup functionality of the plugin will usually like to have internal access to the PluginProcessing object, but it can't convert the trait object back into the original type even if it knows the type.

However, guarding the resume and suspend methods this way is actually not necessary for safety. It is part of the VST concurrency rules that the host is not allowed to call processing methods on a suspended plugin or setup methods on a resumed plugin, and the plugin might very well be unprepared for such calls, but it is strictly not unsafe (per the Rust definition) for it to do so. Thus, if we restrict ourselves to ensuring safety, we can drop the resume/suspend enforcement.

In that case, we can slice the API in a simpler way. If we group all methods callable on the UI thread while the plugin is resumed together into a shared, immutable object, the processing methods can be moved back into the main trait. I think this will be much more convenient both for the plugin and the host. Thus:

/// Parameter state object shared between the UI and processing threads
trait PluginParameters {
    fn change_preset(&self, preset: i32); // Can be called on the processing thread for automation.
    fn get_preset_num(&self) -> i32;
    fn set_preset_name(&self, name: String);
    fn get_preset_name(&self, preset: i32) -> String;
    fn get_parameter_label(&self, index: i32) -> String;
    fn get_parameter_text(&self, index: i32) -> String;
    fn get_parameter_name(&self, index: i32) -> String;
    fn get_parameter(&self, index: i32) -> f32;
    fn set_parameter(&self, index: i32, value: f32); // Can be called on the processing thread for automation.
    fn can_be_automated(&self, index: i32) -> bool;
    fn string_to_parameter(&self, index: i32, text: String) -> bool;
    fn get_preset_data(&self) -> Vec<u8>;
    fn get_bank_data(&self) -> Vec<u8>;
    fn load_preset_data(&self, data: &[u8]);
    fn load_bank_data(&self, data: &[u8]);

    /// Return handle to plugin editor if supported.
    /// The method needs only return the object on the first call.
    /// Subsequent calls can just return `None`
    fn get_editor(&self) -> Option<Box<dyn Editor>>;
}

trait Plugin {
    /// Get a handle to the parameter state object.
    fn get_parameter_object(&mut self) -> Arc<dyn PluginParameters>;

    // All other methods as in the current Plugin trait
}

With this structure, the plugin object belongs to the processing thread while processing is ongoing. If the host wants to suspend the plugin and call some setup methods on it from the UI thread (to change the sample rate, for instance), it will need to either transfer ownership by sending the object between the threads, or share it with an Arc and ensure exclusive access dynamically.

It might look cumbersome that all of the parameter methods have only shared access to the underlying object, but since these will most likely need access to the parameter state accessed by the automation methods, it will need to access shared state in any case, so putting them all together will just be more convenient.

[askeksa]

A re-arrangement of the API according to the latest suggestion can be seen here: askeksa@2c3cf00

It falls into place pretty nicely, I think. The only example that needed any changes was the dimension_expander, since it is the only one that uses parameters. And even that one didn't require many changes apart from the re-arrangement and the actual synchronization code.

What do you think? Does this change look reasonable?

[zyvitski]

You should open a PR and we can take a deeper look into the changes

[askeksa]

I will. First I need to rebase it onto the latest changes (all the clippy/rustfmt stuff is going to be "fun" to merge). Then I will try to port Oidos to the new API to verify that it works well in practice.

Another good verification could be to check how well @Boscop's easyvst crate can be adapted. Ideally it will hide some of the complexity arising from separating out the parameter API.

[raphlinus]

Chiming in here to vote for this bug and thank @askeksa for the analysis and exploration.

The proposed API sounds reasonable to me, and I think is safe. There are a couple other ways to get similar results, but this is probably the cleanest.

In my google/synthesizer-io codebase, I have a lock-free queue abstraction that's designed to be rigorously lock-free (including no allocations) in the processing thread. However, it's going to need some adapting, as it's spsc, and I think it needs to be mpsc because there are at least two threads that can generate messages. This is, I believe, the best way to achieve ergonomics when the plugin is doing complex things. For simpler things (just setting a float parameter), direct use of atomics is reasonable.

I'm very excited about the possibility of getting this stuff right.

[askeksa]

Thanks for the comment, @raphlinus. I have also been thinking about how to construct an appropriate queue or rwlock-like data structure for this use case. I think it needs to be quite specialized because of the special situation. In particular:

• It is not just mpsc; the two producers write to it using the same handle (not cloned handles).
• Since set_parameter can be called from the processing thread, writing needs to be lock-free and allocation free when it is the processing thread that is writing. Since we don't know if we are on the processing thread, this reduces to the requirement that writing must be lock-free and allocation free when not contended with reading, even when contended with a write on the other thread.
• When a write is followed by a read on the same thread, the written message must reach the read (i.e. some kind of eagerness guarantee), otherwise automation will break.

I haven't yet found a surefire way of constructing such a data structure, but it ought to be possible to achieve, if not exactly this, then at least something useful.

[raphlinus]

@askeksa The design of a proper nonblocking queue is probably outside the scope of this issue, though it intersects due to the goal of there being an ergonomic interface. To address your points in order:

• Expressed in types, you mean that the send method is &self and that the sender object is Sync? I think this can easily be accommodated using a compare-and-exchange linked list with some solution for the ABA problem.

• This is the most difficult problem to address in my architecture, which is based on asymmetry between two sides of the channel - one side (the sender in this case) does all the allocation and is allowed to block, the other is rigorously nonblocking. The solution that comes to mind is to have a pool of set_parameter message objects that circulates, but this is tricky. Among other things, what to do when the pool is exhausted?

• Almost all lock-free data structures I can think of will meet this requirement - reads following writes on the same thread will see those writes in order.

It would be easier to just use an atomic float for parameters, but one of my goals is to smooth inputs (and I already have a smoothing filter which behaves very nicely).