atomig icon indicating copy to clipboard operation
atomig copied to clipboard

Require `PrimitiveAtom::Impl` to be `Send + Sync`

Open agerasev opened this issue 1 year ago • 1 comments

Atomic types naturally should be Send + Sync but PrimitiveAtom::Impl does not have these bounds that leads to necessity to write additional where-clauses anywhere Atomic<T> is used.

Maybe these bounds should be added? This would make the usage of this crate much more convenient.

agerasev avatar Aug 18 '24 04:08 agerasev

Hi! Could you give me a bit more detail about a use case where this is important? The Impl associated type used to not be part of the public API. That changed in #11, with me not being super happy about that particular change. My intention was to keep the impls module as hidden as possible.

Apart from Send and Sync, one could also add Debug + From<Self> + Default, since all the std atomic types implement that. But again... that would expose a lot more about the internals and limits me in making changes later on (like adding new impls). So mh, I'm not fully convinced yet, that's why I am asking for use cases.

LukasKalbertodt avatar Oct 10 '24 08:10 LukasKalbertodt

Here is a simple example:

use atomig::{Atom, Atomic};
use std::{
    sync::{atomic::Ordering, Arc},
    thread,
};

fn spawn<T: Atom + Send + 'static>(atomic: Arc<Atomic<T>>) {
    thread::spawn(move || atomic.load(Ordering::Acquire));
}

fn main() {
    spawn(Arc::new(Atomic::new(true)));
    spawn(Arc::new(Atomic::new(1u32)));
}

Compiling this code failed with:

error[E0277]: `<<T as Atom>::Repr as PrimitiveAtom>::Impl` cannot be shared between threads safely

This can only compile with an ugly where clause added:

fn spawn<T: Atom + Send + 'static>(atomic: Arc<Atomic<T>>)
where
    <<T as Atom>::Repr as PrimitiveAtom>::Impl: Send + Sync,

agerasev avatar Oct 16 '24 14:10 agerasev

Fixed in https://github.com/LukasKalbertodt/atomig/commit/92b2019e4a61f2fe0baebe50ad19e92d131a2acc and released as 0.4.2

That's an interesting case. Whether Atomic<T> implements Send or Sync depended on whether <<T as Atom>::Repr as PrimitiveAtom>::Impl does. That's also the error you got. That's not great, Atomic<T> should always implement those traits. So I now simply did what you suggested, and added the trait bound. I also added a test to make sure Atomic<T> always implements Send and Sync as long as T: Atom . As an aside, another solution in your original problem would be to add the bound where Atomic<T>: Send + Sync. A bit nicer, but anyway, not necessary anymore now.

LukasKalbertodt avatar Oct 18 '24 15:10 LukasKalbertodt