Thread safe double buffer implementation

Hi! I recently implemented a double buffer in rust (pretty much a generic swap chain), and I was wondering if someone could look it over. It's the first time I've used unsafe rust before, and I wanted to make sure that it was thread safe. I'm also not sure if the way I implemented it is ergonomic--I implemented a BufferLeft and BufferRight which both have the common trait of BufferHalf. The code can be found here on github (I'll put it below too): https://github.com/wwtos/mjuo/blob/sampling/vpo-backend/sound-engine/src/sampling/double_buffer.rs

use std::{
    ops::DerefMut,
    sync::{Mutex, TryLockError},
};

#[cfg(test)]
use std::{sync::Arc, thread, time::Duration};

pub struct DoubleBuffer<T> {
    pub active: Mutex<T>,
    pub background: Mutex<T>,
}

#[derive(Debug)]
pub struct WouldBlock;

impl<T> DoubleBuffer<T> {
    pub fn new(buffer_left: T, buffer_right: T) -> DoubleBuffer<T> {
        DoubleBuffer {
            active: Mutex::new(buffer_left),
            background: Mutex::new(buffer_right),
        }
    }
}

impl<T> DoubleBuffer<T> {
    pub fn swap(&self) {
        // ensure we have access to these
        let mut lock_active = self.active.lock().unwrap();
        let mut lock_background = self.background.lock().unwrap();

        std::mem::swap(lock_active.deref_mut(), lock_background.deref_mut());
    }

    pub fn try_swap(&self) -> Result<(), WouldBlock> {
        // ensure we have access to these
        let mut lock_active = self.active.try_lock().map_err(|err| match err {
            TryLockError::Poisoned(_) => panic!("poisoned lock"),
            TryLockError::WouldBlock => WouldBlock,
        })?;
        let mut lock_background = self.background.try_lock().map_err(|err| match err {
            TryLockError::Poisoned(_) => panic!("poisoned lock"),
            TryLockError::WouldBlock => WouldBlock,
        })?;

        std::mem::swap(lock_active.deref_mut(), lock_background.deref_mut());

        Ok(())
    }
}

pub fn double_buffer<T>(buffer_a: T, buffer_b: T) -> DoubleBuffer<T>
where
    T: Send + Sync,
{
    DoubleBuffer::new(buffer_a, buffer_b)
}

#[test]
fn test_swap_block() {
    let double_buffer = Arc::new(double_buffer(vec![1, 2, 3], vec![4, 5, 6]));
    let double_buffer_clone = double_buffer.clone();

    let active_lock = (*double_buffer_clone).active.lock().unwrap();
    let active_lock_ref = active_lock.as_ref();

    thread::scope(|s| {
        s.spawn(|| {
            // make sure we can still access the background buffer
            assert!((*double_buffer).background.try_lock().is_ok());

            // now, swapping it should fail
            // (*double_buffer).try_swap();
            assert!((*double_buffer).try_swap().is_err());
        });
    });

    // keep left lock in scope
    assert_eq!(active_lock_ref, vec![1, 2, 3]);
}

#[test]
fn test_swap_lock() {
    let double_buffer = Arc::new(double_buffer(vec![1, 2, 3], vec![4, 5, 6]));
    let double_buffer_clone = double_buffer.clone();

    thread::spawn(move || {
        // lock the right and do something intensive
        let mut background_lock = (*double_buffer_clone).background.lock().unwrap();
        thread::sleep(Duration::from_millis(100));

        background_lock[0] = 7;
        background_lock[1] = 8;
        background_lock[2] = 9;
    });

    // give the other thread a second to lock
    thread::sleep(Duration::from_millis(10));

    // swapping should block
    (*double_buffer).swap();

    assert_eq!((*double_buffer).active.lock().unwrap().as_ref(), vec![7, 8, 9]);
}

Thank you so much!

1 Like

Can you explain why you use UnsafeCell? Have you ruled out safe higher-level abstractions?

I suppose I haven't, lol. I was modelling it based on rust's implementation of mpsc::channel. I might just be able to drop the cell entirely, let me check really fast...

Looks like I didn't need the UnsafeCell after all, thank you for pointing that out!

Comments:

  • Having the locking and swapping functionality duplicated in two types doesn't smell right. Try moving as much code as you can up to DoubleBuffer. You can probably get rid of the split types and the trait.

  • What's the purpose of BufferWrapper? It looks unnecessary.

  • When you have both foo and try_foo it implies that try_foo returns a Result and can fail while foo "just works" (but might block). Don't bother returning poison errors; just unwrap them and panic.

I'd aim for something like:

pub struct DoubleBuffer<T> {
    pub left: Mutex<T>,
    pub right: Mutex<T>,
}

impl<T> DoubleBuffer<T> {
    pub fn swap(&mut self) { ... }
    pub fn try_swap(&mut self) -> Result<(), _> { ... }
}

No extra types for the two buffers if you can avoid it. You call swap on the double buffer as a whole, not the individual buffers. No lock methods; the mutexes are public, you don't need to provide your own.

Further refinement: indicate which buffer is active. Change left/right to active/inactive or primary/secondary.

1 Like

Good point on the duplicated types--that was bothering me too but I wasn't sure what to do. Making the mutexes public makes a lot of sense, the swap code will ensure they're both available, so I don't need to worry about them being private. It also reduces the methods I would need to implement.

I created BufferWrapper when std::mem::swap(&lock_left, &lock_right); wasn't working (I was swapping the mutex guards, not the data), but I now realize I can use deref_mut() to get a reference to the data behind the mutex.

I didn't think about that regarding panicking poison errors--because if a thread has panicked there's something wrong, and I shouldn't try to error recover in that case. Makes a lot of sense!

Again, thank you for this wonderful advice, it's drastically simplified it! I updated the code in the first post as well :slight_smile:

2 Likes

std::sync::mpsc has a somewhat poor reputation. There have been thoughts of replacing it with another channel implementation, such as crossbeam-channel (ticket) or flume (ticket). I like flume, personally.

Huh, reading about the issues with std::sync::mpsc is really interesting, I had no idea something in the standard library would have those issues! I was planning on using a mpsc channel for other parts of my project, but I'll probably use one of those instead.