polyfractal/bounded-spsc-queue

Bad sync impl

mooman219 opened this issue · 1 comments

The line in question:
unsafe impl<T: Sync> Sync for Buffer<T> {}

So, as far as I know this should line should be removed. Buffer inherits !Send and !Sync from the *mut T. Marking it as Sync is unsafe because it requires Buffer to implement thread safety itself, from any number of threads accessing any of its functions. Buffer itself is obviously not safe to be marked as Sync because try_push/try_pop for example will have inconsistencies if multiple threads call either one at once. Buffer's logic only guarantees one thread calling the consumer side (try_pop) of things and one thread calling the producer side (try_push) of things.

Arc is only Send and Sync if the underlying type being wrapped is also Send and Sync. The unsafe Sync marker on Buffer does not change the auto traits of Arc because since Buffer is not Send, Arc can be neither Send or Sync.

On top of all of this, Buffer should not be marked as pub in the first place since you cannot directly access it anyway.

So I'm thinking the correct way to do this is to:

  • have Buffer be both Send and Sync
  • have Buffer be a private type which the user can't access directly, only through methods on Producer and Consumer
  • have both Producer and Consumer only be Send, and explicitly !Sync.

This way you can share the endpoints between two different threads (Buffer is Sync), but only one thread can hold a given endpoint at a time (both endpoints are !Sync).