Skip to content

Conversation

@irh
Copy link

@irh irh commented Oct 20, 2025

This PR changes public Producer/Consumer methods that take &mut self unnecessarily to take &self instead.

The motivation for this change is to simplify client-side use of Producer and Consumer, allowing them to be used from immutable contexts without additional steps.

@mgeier
Copy link
Owner

mgeier commented Oct 20, 2025

Thanks for this PR!

However, I'm afraid the mut is load-bearing and this PR makes the library unsound.

I've created an example that shows the problem:

fn main() {
    let (p, c) = rtrb::RingBuffer::new(1);
    p.push(10).unwrap();
    let reference = c.peek().unwrap();
    assert_eq!(reference, &10);
    let value = c.pop().unwrap();
    assert_eq!(value, 10);
    p.push(666).unwrap();
    assert_eq!(reference, &10);
}

While we have a reference into the buffer, we can overwrite the buffer, which definitely makes the Rust gods angry.

Miri doesn't like it either:

error: Undefined Behavior: trying to retag from <816> for SharedReadOnly permission at alloc255[0x0], but that tag does not exist in the borrow stack for this location
    --> [...]/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cmp.rs:2090:27
     |
2090 |             PartialEq::eq(*self, *other)
     |                           ^^^^^ this error occurs as part of retag at alloc255[0x0..0x4]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <816> was created by a SharedReadOnly retag at offsets [0x0..0x4]
    --> examples/mut.rs:4:21
     |
   4 |     let reference = c.peek().unwrap();
     |                     ^^^^^^^^^^^^^^^^^
help: <816> was later invalidated at offsets [0x0..0x4] by a write access
    --> [...]/src/lib.rs:324:22
     |
 324 |             unsafe { self.buffer.slot_ptr(tail).write(value) };
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If you try it on the main branch, this example doesn't compile (which is expected).

Note that this doesn't only happen for peek(), but also for the "chunk" functions (but I haven't actually tested it).

@irh
Copy link
Author

irh commented Oct 20, 2025

Thanks for the explanation, I'm not sure why that didn't jump out at me. Sorry for the noise!

@mgeier
Copy link
Owner

mgeier commented Oct 21, 2025

No problem at all, this subtlety is easy to miss.

I thought about how this could be made clearer in the docs, and my only idea was #148.

Would that help?

@irh
Copy link
Author

irh commented Oct 21, 2025

Thanks, yes that highlights the importance of push/pop taking &mut self, and having an example that would break if someone else starts experimenting in this direction seems like a good idea 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants