Skip to content

BufferIterator can be Send + Sync #8873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
robert3005 merged 1 commit into from
Jul 23, 2026
+16 −0
Merged

BufferIterator can be Send + Sync #8873

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions vortex-buffer/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,11 @@ pub struct BufferIterator<T: Copy> {
end: *const T,
}

// SAFETY: `BufferIterator` is a `Buffer<T>` plus two cursors into it, so it can safely be

@0ax1 0ax1 Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the compiler auto-derive Send + Sync if T is so? Right bc of the pointer

// `Send`/`Sync` exactly when `Buffer<T>` is. Same bounds as `std::vec::IntoIter`.
unsafe impl<T: Copy + Send> Send for BufferIterator<T> {}

@0ax1 0ax1 Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude Can't we copy the iterator and share across threads such that it aliases and races?

@github-actions github-actions Bot Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @0ax1's task in 57s —— View job

Re: "Can't we copy the iterator and share across threads such that it aliases and races?"

Short answer: no, this can't cause a data race. A few reasons stack up:

1. BufferIterator is neither Copy nor Clone.
There's no derive and no manual impl (buffer.rs:709-714), so you literally cannot duplicate it. To move it to another thread you have to move it (Send), which leaves nothing behind on the original thread.

2. Even if it were cloneable, there'd still be no race.
The iterator only ever reads from the buffer — next() does self.ptr.read() (a copy out) and advances its own cursor (buffer.rs:725-733). It never writes into the backing Buffer<T>. Two independent iterators over the same buffer are just two concurrent readers of shared immutable memory, which is exactly what Buffer<T>: Sync already guarantees is safe (same as cloning a slice::Iter or holding two Arc readers).

3. Sync only exposes &self methods, none of which mutate.
Sync means &BufferIterator can be shared across threads. The only thing you can do through a shared & is call size_hint() (buffer.rs:736-740), which reads ptr/end immutably. Advancing the cursor requires next(&mut self), i.e. exclusive access — the borrow checker forbids two threads holding &mut to the same iterator simultaneously. So the aliasing-mutation-race scenario can't be constructed safely.

4. The bounds match the standard library.
T: Send/T: Sync are exactly the bounds std::vec::IntoIter<T> and slice::Iter<'_, T> use. The manual impls are needed only because the two raw pointers (*const T) opt the struct out of the auto-derive (which 0ax1 already noted above); the Send/Sync semantics of the underlying data are unchanged.

So the impls are sound: the cursors are owned per-iterator, the buffer is read-only during iteration, and mutation is gated behind &mut. This is the same reasoning that lets Vec::into_iter() be Send + Sync.

@0ax1 0ax1 Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude this is sound then?

@github-actions github-actions Bot Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @0ax1's task in 30s —— View job

Yes — this is sound. ✅

I re-read the current code at vortex-buffer/src/buffer.rs:709-740 and the reasoning from my earlier reply holds:

1. The bounds match std exactly.

unsafe impl<T: Copy + Send> Send for BufferIterator<T> {}
unsafe impl<T: Copy + Sync> Sync for BufferIterator<T> {}

These are the same bounds std::vec::IntoIter<T> (Send/Sync iff T) and slice::Iter<'_, T> use. The manual unsafe impls are only needed because the two *const T cursors (lines 712-713) opt the struct out of the auto-derive — exactly as you noted. They don't change the Send/Sync semantics of the underlying data.

2. No aliasing is possible.
BufferIterator is neither Copy nor Clone (no derive, no manual impl). To get it onto another thread you must move it (Send), which leaves nothing behind. You cannot end up with two cursors over the same buffer from a single iterator.

3. Iteration is read-only.
next() (lines 725-733) does self.ptr.read() (a copy-out, valid because T: Copy) and advances its own owned cursor. It never writes into the backing Buffer<T>. Even two independent iterators over the same buffer would just be two concurrent readers of shared immutable memory — already guaranteed safe by Buffer<T>: Sync.

4. Sync only exposes non-mutating &self access.
Through a shared &BufferIterator the only reachable method is size_hint() (lines 736-740), which reads ptr/end immutably. Advancing requires next(&mut self), i.e. exclusive access — the borrow checker forbids two threads holding &mut to the same iterator, so the alias-and-race scenario can't be constructed in safe code.

One optional nit (not a soundness issue): size_hint uses unsafe { self.end.offset_from(self.ptr) } without a // SAFETY: comment, while the other unsafe blocks in this struct have one. Adding a short justification (both pointers derive from the same allocation, end >= ptr is an invariant) would keep it consistent with the surrounding style.

So: the impls are sound, and equivalent to why Vec::into_iter() is Send + Sync.

unsafe impl<T: Copy + Sync> Sync for BufferIterator<T> {}

impl<T: Copy> Iterator for BufferIterator<T> {
type Item = T;

Expand Down Expand Up @@ -777,6 +782,17 @@ mod test {
assert_eq!(aligned.as_slice(), &[0, 1, 2]);
}

#[test]
fn buffer_iterator_send_sync() {
fn assert_send_sync<T: Send + Sync>(_: &T) {}

let mut iter = buffer![0i32, 1, 2, 3].into_iter();
assert_send_sync(&iter);
iter.next();
let remaining: Vec<i32> = std::thread::spawn(move || iter.collect()).join().unwrap();
assert_eq!(remaining, vec![1, 2, 3]);
}

#[test]
fn slice() {
let buf = buffer![0, 1, 2, 3, 4];
Expand Down
Loading