Review Request for a Thread-Safe ID-String Pair Structure in Axum

Hello Rustaceans,

I'm currently working on a project using the Axum server and I need to create a structure to track a list of ID-string pairs. The order of the pairs is important and should be preserved as they are inserted.

Here's the structure I've come up with:

use anyhow::{anyhow, Result};
use std::collections::VecDeque;
use std::sync::{Arc, Mutex};

struct ThreadSafeQueue<T> {
    queue: Arc<Mutex<VecDeque<T>>>,
    max_size: usize,
}

impl<T> ThreadSafeQueue<T>
where
    T: Clone,
{
    fn new(max_size: usize) -> Self {
        ThreadSafeQueue {
            queue: Arc::new(Mutex::new(VecDeque::new())),
            max_size,
        }
    }

    fn insert(&self, item: T) -> Result<()> {
        let mut queue = self
            .queue
            .lock()
            .map_err(|_| anyhow!("Failed to acquire lock"))?;
        if queue.len() == self.max_size {
            queue.pop_front(); // Remove the oldest element
        }
        queue.push_back(item);
        Ok(())
    }

    fn iterate(&self) -> Result<Vec<T>> {
        let queue = self
            .queue
            .lock()
            .map_err(|_| anyhow!("Failed to acquire lock"))?;
        Ok(queue.clone().into_iter().collect())
    }
}

This structure is fairly simple, but I want to make sure it’s thread-safe and efficient. Could you please review my code and suggest any improvements or changes if necessary?

Thank you in advance for your help!

It's definitely thread-safe. That's an easy one since Rust guarantees this when there is no unsafe code.

The insert method is very efficient because you're treating the VecDeque as a fixed sized circular list. And you're moving in the item parameter, so it isn't cloned unless the caller clones it.

The iterate method is doing an extra clone of the VecDeque backing array. The following (what you're doing) clones the entire VecDeque including the backing array and the elements. Then it iterates over the elements and collects them into a Vec.

        Ok(queue.clone().into_iter().collect())`

You can avoid the clone of the backing array as shown below. This iterates over the elements, clones them, and collects them into a Vec.

        Ok(queue.iter().cloned().collect())

A potentially even more efficient approach is to clone the VecDeque as you're already doing, but then transform that clone into a Vec using the Vec::from(VecDeque) as follows. This does not clone the backing array again, it reuses it. Instead of iterating all the elements, this moves them around in the backing array but only if necessary to produce a front-to-back order, so this could potentially be more efficient that iterating and moving all of them.

        Ok(queue.clone().into())

Also be aware that iterate is cloning all the elements and you said it contains Strings, so if it is large this would be do a lot of String allocations. Whether this is a problem depends on how you're using it.

I would definitely recommend renaming iterate since it clones the elements and doesn't return an Iterator. Perhaps make_vec? I'm not thinking of very good names myself, sorry.

2 Likes

You do not need to handle the mutex lock error. It only occures if a thread panicked while holding the lock which can only happen in OOM situations in your code. Furthermore, the inner VecDeque will always be in a valid state. So you can either unwrap or unwrap_or_else(|err| err.into_inner()).

1 Like