Skip to content
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.

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

Pinned synchronization primitives #3124

Closed
wants to merge 5 commits into from
Closed

Conversation

nicbn
Copy link

@nicbn nicbn commented May 14, 2021

This RFC aims to provide std::sync::pinned module, with synchronization primitives which are !Unpin. These are better used when something like Arc<Mutex<T>> is needed, as they avoid an extra allocation on some platforms, e.g. Linux.

Previous discussion on internals forum

Rendered

@thomcc
Copy link
Member

thomcc commented May 24, 2021

It would be nice if there were an unsafe way of initializing them without requiring &mut, since they already have interior mutability, and fabricating a &mut Mutex<T> would be unsound if you e.g. only have a static Mutex.

@shepmaster
Copy link
Member

Can this be done in a crate to prove it out first, and allow people to start using it?

@nicbn
Copy link
Author

nicbn commented May 24, 2021

@thomcc (editted) Sorry, I made confusion. The structure does contain interior mutability for platforms such as posix.

So I guess this is up for debate, but in my opinion it makes more sense to use a wrapper with UnsafeCell or static mut still, as the flag for the initialization itself does not need interior mutability.

struct Mutex {
    inner: UnsafeCell<libc::pthread_mutex_t>,
    initialized: bool,
}

In the example above, initialized does not need interior mutability if we don't have unsafe unchecked_init(&self). I think not having interior mutability here could help for optimizations but I'm not sure.

Also, there is a third solution, which is to make a safe init(&self) that works by keeping track of initialization via an atomic flag and panicking in double initialization as well as usage before initialization.

@shepmaster I will write a crate for this.

@nicbn
Copy link
Author

nicbn commented Jun 3, 2021

While writing the tests, I realized that by making init take a mutable reference, it is impossible to initialize e.g. Pin<Arc<(Mutex<T>, Condvar)>> safely.

unsafe {
    let mut this = Arc::new((Mutex::uninit(value), Condvar::uninit()));
    let this_ref = Arc::get_mut(&mut this).unwrap();
    Pin::new_unchecked(&mut this_ref.0).init();
    Pin::new_unchecked(&mut this_ref.1).init();
    Pin::new_unchecked(this) // Pin<Arc<(Mutex<T>, Condvar)>>
}

This is pretty much a dealbreaker, as it locks the major benefit of the change behind unsafety. Therefore, I will change the RFC so that initialization is done by shared references.

@nicbn
Copy link
Author

nicbn commented Jun 8, 2021

The RFC has been updated to reflect the change in init.

New rationale and alternatives have been added as well.

Sorry for taking this long, I'm finishing the crate and will update here when it's done

@nikomatsakis
Copy link
Contributor

I agree that exploring this in the crates.io ecosystem first seems like a good idea.

@nicbn
Copy link
Author

nicbn commented Jul 1, 2021

So, there's another problem with the initialization pattern. Initializing Pin<Arc<(Mutex<T>, Condvar)>> safely is not possible even if init takes Pin<&Self>. That's because there's no way to get a projection using safe code only.

This breaks the argument I made above for init taking a exclusive reference being a dealbreaker, however I'll be keeping it for the added ergonomics of initializing static and Arc easily.

The crate has been published and is available at https://github.com/nicbn/pinned-sync

@ehuss ehuss added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 5, 2021
jenisys added a commit to jenisys/rust-lang_rfcs that referenced this pull request Jan 6, 2022
…n macOS

* Omit using "ln -r" for symbolic link creation
* Create symbolic links with old style expression
* Creates relative links
@nicbn
Copy link
Author

nicbn commented Apr 23, 2022

From rust-lang/rust#93740, currently we are moving towards another approach for sync primitives. As of now, I personally prefer the new approach, as it will address the problem (boxing the primitives) without the cons of Pin ergonomics - therefore I will be closing this RFC. But if anyone else takes interest, feel free to use anything of the text you'd like.

@nicbn nicbn closed this Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants