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

Remove the Sync requirement for #[thread_local] statics. #35035

Closed
ticki opened this issue Jul 25, 2016 · 10 comments
Closed

Remove the Sync requirement for #[thread_local] statics. #35035

ticki opened this issue Jul 25, 2016 · 10 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@ticki
Copy link
Contributor

ticki commented Jul 25, 2016

This restriction can be very bothersome, and has no real purpose. thread_locals always belongs to a single thread, thus interior mutability won't cause data races.

@ticki
Copy link
Contributor Author

ticki commented Jul 27, 2016

Here's a temporary workaround:

#![feature(const_fn, thread_local)]

use std::{ops, marker};

/// Add `Sync` to an arbitrary type.
///
/// This primitive is used to get around the `Sync` requirement in `static`s (even thread local
/// ones! see rust-lang/rust#35035). Due to breaking invariants, creating a value of such type is
/// unsafe, and care must be taken upon usage.
///
/// In general, this should only be used when you know it won't be shared across threads (e.g. the
/// value is stored in a thread local variable).
pub struct Syncify<T>(T);

impl<T> Syncify<T> {
    /// Create a new `Syncify` wrapper.
    ///
    /// # Safety
    ///
    /// This is invariant-breaking and thus unsafe.
    const unsafe fn new(inner: T) -> Syncify<T> {
        Syncify(inner)
    }
}

impl<T> ops::Deref for Syncify<T> {
    type Target = T;

    fn deref(&self) -> &T {
        &self.0
    }
}

unsafe impl<T> marker::Sync for Syncify<T> {}

/// Declare a thread-local static variable.
///
/// TLS works by copying the initial data on every new thread creation. This allows access to a
/// variable, which is only available for the current thread, meaning that there is no need for
/// syncronization.
///
/// For this reason, in contrast to other `static`s in Rust, this need not thread-safety, which is
/// what this macro "fixes".
macro_rules! tls {
    (static $name:ident: $ty:ty = $val:expr) => { tls!(#[] static $name: $ty = $val); };
    (#[$($attr:meta),*] static $name:ident: $ty:ty = $val:expr) => {
        $(#[$attr])*
        #[thread_local]
        static $name: Syncify<$ty> = unsafe { Syncify::new($val) };
    }
}

use std::cell::Cell;

tls!(static CELL: Cell<u32> = Cell::new(2));
tls!(static CELL2: Cell<&'static str> = Cell::new("a"));

fn main() {
    CELL.set(3);
}

@ticki
Copy link
Contributor Author

ticki commented Jul 27, 2016

@steveklabnik the A-libs tag is wrong.

@eddyb eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-libs labels Aug 1, 2016
@nikomatsakis
Copy link
Contributor

This seems related to the RFC around signal handlers. Are signal handlers the main reason that there is a sync requirement? I agree it can be burdensome and seems unnecessary, except for that.

cc @alexcrichton

@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang -- thoughts on whether thread-local values must be sync?

@alexcrichton
Copy link
Member

I think this is kinda a duplicate of #17954, I believe the Sync behavior has been brought up there.

AFAIK the Sync requirement is because static variables require Sync, and no one ever got around to making that check conditional on whether #[thread_local] was present or not. That is, I don't believe it's intentional. It makes sense to me to remove it, but it still doesn't make the feature sound (ala #17954)

@nikomatsakis
Copy link
Contributor

Ah yeah I'd forgotten completely about #17954.

@ticki
Copy link
Contributor Author

ticki commented Aug 6, 2016

rust-lang/rfcs#1705

@Mark-Simulacrum
Copy link
Member

So I think discussion above seems to indicate that we should close this as a somewhat-duplicate of #17954, so I'm going to close in favor of it.

@eddyb
Copy link
Member

eddyb commented May 11, 2017

It's not a duplicate though.

@eddyb eddyb reopened this May 11, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 25, 2017
@alexcrichton
Copy link
Member

Closed by #43746

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants