AudioNode ergonomics: atomics, interior mutability, Send/Sync?
orottier opened this issue · 4 comments
Continuing discussion from #342
For historical reasons, many of the audio node properties are stored as atomics.
This results in all audio node methods taking &self instead of requiring &mut self.
As pointed out in the linked discussion, the usage of atomic may result in logic errors and should thus be rewritten.
Replacing the atomics by regular values will change the signature of all methods to &mut.
This is not bad per se, but is a breaking change and does require some rewrites on https://github.com/ircam-ismm/node-web-audio-api
Alternatively, we can leave the signatures unchanged but that will require to implement the interior mutability ourselves:
- via RefCell, which is low overhead but marks the nodes not being Send/Sync, which I think is a big disadvantage [1]
- via Mutex, which I think is very much overkill. Users that need to update the node from multiple thread can choose to do that themselves instead.
My preference would be to move all methods to &mut and make the necessary adjustments to the node bindings.
[1] this prevents e.g. a usage pattern where a thread is spawned to handle keyboard input which controls an AudioBufferSourceNode directly
Apparently I already test some structs to be Send + Sync + 'static https://github.com/orottier/web-audio-api-rs/blob/main/tests/online.rs#L26
We should probably test this for all possible nodes, to prevent accidental regression (like the AudioBufferSource node using OnceCell instead of OnceLock)
I don't think the node bindings should be actually considered part of the discussion.
Except very weird unexpected problem, that's probably just adding some mut here and there (and the probably related clippy::please_shutup_about_non_required_mut directive)
Alright, then I think it is best to not use interior mutability and mark all methods &mut when required.
This is idiomatic, sound and will not be a burden in almost all use cases.
The node bindings will need a slight update. Most nodes are currently stored for napi as Rc<...Node>. This will need to be Rc<RefCell<...Node>> and we will need a bunch of borrow_mut calls here and there.
I will start migrating some nodes to the new style