lylythechosenone / no-std-async

Async synchronization primitives for #[no_std] rust
1 stars 1 forks source link

Potentially incorrect implementation of `RwLock` #1

Closed Finomnis closed 11 months ago

Finomnis commented 12 months ago
    pub const fn new(data: T) -> Self {
        Self {
            data: UnsafeCell::new(data),
            semaphore: Semaphore::new(usize::MAX),
            max_readers: 0,
        }
    }

    /// Acquires a write lock on the data.
    pub async fn write(&self) -> WriteGuard<'_, T> {
        self.semaphore.acquire(self.max_readers).await;
        WriteGuard { rwlock: self }
    }

max_readers: 0 together with self.semaphore.acquire(self.max_readers) means that aquiring a write lock is actually a no-op and doesn't perform any locking.

Are you sure it shouldn't be this?

    pub const fn new(data: T) -> Self {
        Self {
            data: UnsafeCell::new(data),
            semaphore: Semaphore::new(usize::MAX),
            max_readers: usize::MAX,
        }
    }

Or even simpler:

    pub const fn new(data: T) -> Self {
        Self::with_max_readers(data, usize::MAX)
    }

If this is the case, then I also recommend many more automated tests, so that the confidence in the correctness of this crate is improved.

lylythechosenone commented 11 months ago

Ah, you are entirely right! Will push a fix. (I'm actually amazed anybody uses this crate, it was just a util crate for my hobby OS lol)

Finomnis commented 11 months ago

Well, it's published and has a catchy name :) async programming on embedded is kinda new, but Rtic 2 is competent based on it. And other sync primitive like Tokio are std. So your crate might experience some more traffic in future.

If this is the case, I would indeed recommend some more tests :) errors like this shouldn't be able to slip through.

lylythechosenone commented 11 months ago

Yeah, I'll add tests as soon as I have time (probably this weekend), and publish a new version.