From 42094f3f42c56b8363d12f94d0e4c89963c728df Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 2 Apr 2015 08:33:44 +1100 Subject: [PATCH] Add Send/Sync bounds to many generic concurrent types. These types are purely designed for concurrent code, and, at the moment, are restricted to be used in those situations. This solidifies that goal by imposing a strict restriction on the generic types from the start, i.e. in the definition itself. This is the opposite to #23176 which relaxes the bounds entirely (it is backwards compatible to switch to that more flexible approach). Unfortunately the message-passing primitives in std (the return value from a thread, and sync::mpsc) aren't great about how they work with mutability and sharing, and so require hacky error-prone `unsafe impl`s. However, they are purely implementation details: the interface isn't affected by having to make that internal change, and clean-ups to the code should be able to remove the hacks. --- src/liballoc/arc.rs | 40 ++++++++++++++-------------- src/libcollections/borrow.rs | 4 +-- src/libserialize/serialize.rs | 2 +- src/libstd/io/lazy.rs | 4 +-- src/libstd/sync/mpsc/mod.rs | 50 ++++++++++++++++++++++++----------- src/libstd/sync/mutex.rs | 2 ++ src/libstd/sync/rwlock.rs | 4 +-- src/libstd/thread/mod.rs | 20 ++++++++------ 8 files changed, 76 insertions(+), 50 deletions(-) diff --git a/src/liballoc/arc.rs b/src/liballoc/arc.rs index 855c86f08e745..7c80f695f681b 100644 --- a/src/liballoc/arc.rs +++ b/src/liballoc/arc.rs @@ -119,7 +119,7 @@ use heap::deallocate; /// ``` #[unsafe_no_drop_flag] #[stable(feature = "rust1", since = "1.0.0")] -pub struct Arc { +pub struct Arc { // FIXME #12808: strange name to try to avoid interfering with // field accesses of the contained type via Deref _ptr: NonZero<*mut ArcInner>, @@ -136,7 +136,7 @@ unsafe impl Sync for Arc { } #[unsafe_no_drop_flag] #[unstable(feature = "alloc", reason = "Weak pointers may not belong in this module.")] -pub struct Weak { +pub struct Weak { // FIXME #12808: strange name to try to avoid interfering with // field accesses of the contained type via Deref _ptr: NonZero<*mut ArcInner>, @@ -146,13 +146,13 @@ unsafe impl Send for Weak { } unsafe impl Sync for Weak { } #[stable(feature = "rust1", since = "1.0.0")] -impl fmt::Debug for Weak { +impl fmt::Debug for Weak { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "(Weak)") } } -struct ArcInner { +struct ArcInner { strong: atomic::AtomicUsize, weak: atomic::AtomicUsize, data: T, @@ -161,7 +161,7 @@ struct ArcInner { unsafe impl Send for ArcInner {} unsafe impl Sync for ArcInner {} -impl Arc { +impl Arc { /// Constructs a new `Arc`. /// /// # Examples @@ -205,7 +205,7 @@ impl Arc { } } -impl Arc { +impl Arc { #[inline] fn inner(&self) -> &ArcInner { // This unsafety is ok because while this arc is alive we're guaranteed @@ -235,15 +235,15 @@ impl Arc { /// Get the number of weak references to this value. #[inline] #[unstable(feature = "alloc")] -pub fn weak_count(this: &Arc) -> usize { this.inner().weak.load(SeqCst) - 1 } +pub fn weak_count(this: &Arc) -> usize { this.inner().weak.load(SeqCst) - 1 } /// Get the number of strong references to this value. #[inline] #[unstable(feature = "alloc")] -pub fn strong_count(this: &Arc) -> usize { this.inner().strong.load(SeqCst) } +pub fn strong_count(this: &Arc) -> usize { this.inner().strong.load(SeqCst) } #[stable(feature = "rust1", since = "1.0.0")] -impl Clone for Arc { +impl Clone for Arc { /// Makes a clone of the `Arc`. /// /// This increases the strong reference count. @@ -277,7 +277,7 @@ impl Clone for Arc { } #[stable(feature = "rust1", since = "1.0.0")] -impl Deref for Arc { +impl Deref for Arc { type Target = T; #[inline] @@ -324,7 +324,7 @@ impl Arc { #[unsafe_destructor] #[stable(feature = "rust1", since = "1.0.0")] -impl Drop for Arc { +impl Drop for Arc { /// Drops the `Arc`. /// /// This will decrement the strong reference count. If the strong reference @@ -392,7 +392,7 @@ impl Drop for Arc { #[unstable(feature = "alloc", reason = "Weak pointers may not belong in this module.")] -impl Weak { +impl Weak { /// Upgrades a weak reference to a strong reference. /// /// Upgrades the `Weak` reference to an `Arc`, if possible. @@ -458,7 +458,7 @@ impl Clone for Weak { #[unsafe_destructor] #[stable(feature = "rust1", since = "1.0.0")] -impl Drop for Weak { +impl Drop for Weak { /// Drops the `Weak`. /// /// This will decrement the weak reference count. @@ -503,7 +503,7 @@ impl Drop for Weak { } #[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq for Arc { +impl PartialEq for Arc { /// Equality for two `Arc`s. /// /// Two `Arc`s are equal if their inner value are equal. @@ -535,7 +535,7 @@ impl PartialEq for Arc { fn ne(&self, other: &Arc) -> bool { *(*self) != *(*other) } } #[stable(feature = "rust1", since = "1.0.0")] -impl PartialOrd for Arc { +impl PartialOrd for Arc { /// Partial comparison for two `Arc`s. /// /// The two are compared by calling `partial_cmp()` on their inner values. @@ -614,21 +614,21 @@ impl PartialOrd for Arc { fn ge(&self, other: &Arc) -> bool { *(*self) >= *(*other) } } #[stable(feature = "rust1", since = "1.0.0")] -impl Ord for Arc { +impl Ord for Arc { fn cmp(&self, other: &Arc) -> Ordering { (**self).cmp(&**other) } } #[stable(feature = "rust1", since = "1.0.0")] -impl Eq for Arc {} +impl Eq for Arc {} #[stable(feature = "rust1", since = "1.0.0")] -impl fmt::Display for Arc { +impl fmt::Display for Arc { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&**self, f) } } #[stable(feature = "rust1", since = "1.0.0")] -impl fmt::Debug for Arc { +impl fmt::Debug for Arc { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Debug::fmt(&**self, f) } @@ -641,7 +641,7 @@ impl Default for Arc { } #[stable(feature = "rust1", since = "1.0.0")] -impl Hash for Arc { +impl Hash for Arc { fn hash(&self, state: &mut H) { (**self).hash(state) } diff --git a/src/libcollections/borrow.rs b/src/libcollections/borrow.rs index 2fe769b73f5cc..b84c2d2ec2bb2 100644 --- a/src/libcollections/borrow.rs +++ b/src/libcollections/borrow.rs @@ -16,7 +16,7 @@ use core::clone::Clone; use core::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd}; use core::convert::AsRef; use core::hash::{Hash, Hasher}; -use core::marker::Sized; +use core::marker::{Send, Sized, Sync}; use core::ops::Deref; use core::option::Option; @@ -115,7 +115,7 @@ impl Borrow for rc::Rc { fn borrow(&self) -> &T { &**self } } -impl Borrow for arc::Arc { +impl Borrow for arc::Arc { fn borrow(&self) -> &T { &**self } } diff --git a/src/libserialize/serialize.rs b/src/libserialize/serialize.rs index 81b5d4c5818b6..9036f892b103d 100644 --- a/src/libserialize/serialize.rs +++ b/src/libserialize/serialize.rs @@ -612,7 +612,7 @@ impl Decodable for RefCell { } } -impl Encodable for Arc { +impl Encodable for Arc { fn encode(&self, s: &mut S) -> Result<(), S::Error> { (**self).encode(s) } diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs index df280dab37d46..7b05f8ac86c57 100644 --- a/src/libstd/io/lazy.rs +++ b/src/libstd/io/lazy.rs @@ -15,13 +15,13 @@ use cell::UnsafeCell; use rt; use sync::{StaticMutex, Arc}; -pub struct Lazy { +pub struct Lazy { pub lock: StaticMutex, pub ptr: UnsafeCell<*mut Arc>, pub init: fn() -> Arc, } -unsafe impl Sync for Lazy {} +unsafe impl Sync for Lazy {} macro_rules! lazy_init { ($init:expr) => (::io::lazy::Lazy { diff --git a/src/libstd/sync/mpsc/mod.rs b/src/libstd/sync/mpsc/mod.rs index e14f32865fa10..19dbf87774f6b 100644 --- a/src/libstd/sync/mpsc/mod.rs +++ b/src/libstd/sync/mpsc/mod.rs @@ -373,12 +373,10 @@ unsafe impl Send for Sender { } /// owned by one task, but it can be cloned to send to other tasks. #[stable(feature = "rust1", since = "1.0.0")] pub struct SyncSender { - inner: Arc>>, + inner: Arc>>, } -unsafe impl Send for SyncSender {} - -impl !Sync for SyncSender {} +impl !Sync for SyncSender {} /// An error returned from the `send` function on channels. /// @@ -434,11 +432,33 @@ pub enum TrySendError { } enum Flavor { - Oneshot(Arc>>), - Stream(Arc>>), - Shared(Arc>>), - Sync(Arc>>), + Oneshot(Arc>>), + Stream(Arc>>), + Shared(Arc>>), + Sync(Arc>>), +} + +// the channel impls are free and fast with mutability and sharing, so +// we have to hack around this with this type. +struct SyncUnsafeCell { + data: UnsafeCell } +impl SyncUnsafeCell { + pub fn new(x: T) -> SyncUnsafeCell { + SyncUnsafeCell { + data: UnsafeCell::new(x) + } + } +} +impl ::ops::Deref for SyncUnsafeCell { + type Target = UnsafeCell; + fn deref(&self) -> &UnsafeCell { + &self.data + } +} + +unsafe impl Send for SyncUnsafeCell {} +unsafe impl Sync for SyncUnsafeCell {} #[doc(hidden)] trait UnsafeFlavor { @@ -489,7 +509,7 @@ impl UnsafeFlavor for Receiver { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn channel() -> (Sender, Receiver) { - let a = Arc::new(UnsafeCell::new(oneshot::Packet::new())); + let a = Arc::new(SyncUnsafeCell::new(oneshot::Packet::new())); (Sender::new(Flavor::Oneshot(a.clone())), Receiver::new(Flavor::Oneshot(a))) } @@ -529,7 +549,7 @@ pub fn channel() -> (Sender, Receiver) { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn sync_channel(bound: usize) -> (SyncSender, Receiver) { - let a = Arc::new(UnsafeCell::new(sync::Packet::new(bound))); + let a = Arc::new(SyncUnsafeCell::new(sync::Packet::new(bound))); (SyncSender::new(a.clone()), Receiver::new(Flavor::Sync(a))) } @@ -576,12 +596,12 @@ impl Sender { let (new_inner, ret) = match *unsafe { self.inner() } { Flavor::Oneshot(ref p) => { unsafe { - let p = p.get(); + let p = p.data.get(); if !(*p).sent() { return (*p).send(t).map_err(SendError); } else { let a = - Arc::new(UnsafeCell::new(stream::Packet::new())); + Arc::new(SyncUnsafeCell::new(stream::Packet::new())); let rx = Receiver::new(Flavor::Stream(a.clone())); match (*p).upgrade(rx) { oneshot::UpSuccess => { @@ -623,7 +643,7 @@ impl Clone for Sender { fn clone(&self) -> Sender { let (packet, sleeper, guard) = match *unsafe { self.inner() } { Flavor::Oneshot(ref p) => { - let a = Arc::new(UnsafeCell::new(shared::Packet::new())); + let a = Arc::new(SyncUnsafeCell::new(shared::Packet::new())); unsafe { let guard = (*a.get()).postinit_lock(); let rx = Receiver::new(Flavor::Shared(a.clone())); @@ -635,7 +655,7 @@ impl Clone for Sender { } } Flavor::Stream(ref p) => { - let a = Arc::new(UnsafeCell::new(shared::Packet::new())); + let a = Arc::new(SyncUnsafeCell::new(shared::Packet::new())); unsafe { let guard = (*a.get()).postinit_lock(); let rx = Receiver::new(Flavor::Shared(a.clone())); @@ -681,7 +701,7 @@ impl Drop for Sender { //////////////////////////////////////////////////////////////////////////////// impl SyncSender { - fn new(inner: Arc>>) -> SyncSender { + fn new(inner: Arc>>) -> SyncSender { SyncSender { inner: inner } } diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index b24cfbb6899a9..6f63f2fe76803 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -170,6 +170,8 @@ pub struct MutexGuard<'a, T: 'a> { __poison: poison::Guard, } +// It is undefined behaviour to unlock a mutex on a thread other than +// the one that locked it. impl<'a, T> !marker::Send for MutexGuard<'a, T> {} /// Static initialization of a mutex. This constant can be used to initialize diff --git a/src/libstd/sync/rwlock.rs b/src/libstd/sync/rwlock.rs index a79ffaa0860e3..a614a4ab2fad7 100644 --- a/src/libstd/sync/rwlock.rs +++ b/src/libstd/sync/rwlock.rs @@ -60,7 +60,7 @@ use fmt; /// } // write lock is dropped here /// ``` #[stable(feature = "rust1", since = "1.0.0")] -pub struct RwLock { +pub struct RwLock { inner: Box, data: UnsafeCell, } @@ -251,7 +251,7 @@ impl RwLock { #[unsafe_destructor] #[stable(feature = "rust1", since = "1.0.0")] -impl Drop for RwLock { +impl Drop for RwLock { fn drop(&mut self) { unsafe { self.inner.lock.destroy() } } diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index 1202b353317cd..656bce79a0231 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -292,7 +292,7 @@ impl Builder { let my_thread = Thread::new(name); let their_thread = my_thread.clone(); - let my_packet = Packet(Arc::new(UnsafeCell::new(None))); + let my_packet = Packet(Arc::new(PacketInner { data: UnsafeCell::new(None) })); let their_packet = Packet(my_packet.0.clone()); // Spawning a new OS thread guarantees that __morestack will never get @@ -331,7 +331,7 @@ impl Builder { } }; unsafe { - *their_packet.0.get() = Some(match (output, try_result) { + *their_packet.0.data.get() = Some(match (output, try_result) { (Some(data), Ok(_)) => Ok(data), (None, Err(cause)) => Err(cause), _ => unreachable!() @@ -585,26 +585,30 @@ impl thread_info::NewThread for Thread { #[stable(feature = "rust1", since = "1.0.0")] pub type Result = ::result::Result>; -struct Packet(Arc>>>); +struct Packet(Arc>); +// see also SyncUnsafeCell in sync/mspc. +struct PacketInner { + data: UnsafeCell>> +} -unsafe impl Send for Packet {} -unsafe impl Sync for Packet {} +unsafe impl Send for PacketInner {} +unsafe impl Sync for PacketInner {} /// Inner representation for JoinHandle and JoinGuard -struct JoinInner { +struct JoinInner { native: imp::rust_thread, thread: Thread, packet: Packet, joined: bool, } -impl JoinInner { +impl JoinInner { fn join(&mut self) -> Result { assert!(!self.joined); unsafe { imp::join(self.native) }; self.joined = true; unsafe { - (*self.packet.0.get()).take().unwrap() + (*self.packet.0.data.get()).take().unwrap() } } }