Skip to content

Commit

Permalink
Use Pin for the 'don't move' requirement of ReentrantMutex.
Browse files Browse the repository at this point in the history
The code in io::stdio before this change misused the ReentrantMutexes,
by calling init() on them and moving them afterwards. Now that
ReentrantMutex requires Pin for init(), this mistake is no longer easy
to make.
  • Loading branch information
m-ou-se committed Dec 8, 2020
1 parent 8fe9096 commit 67c18fd
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 73 deletions.
54 changes: 29 additions & 25 deletions library/std/src/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::cell::{Cell, RefCell};
use crate::fmt;
use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter};
use crate::lazy::SyncOnceCell;
use crate::pin::Pin;
use crate::sync::atomic::{AtomicBool, Ordering};
use crate::sync::{Arc, Mutex, MutexGuard};
use crate::sys::stdio;
Expand Down Expand Up @@ -490,7 +491,7 @@ pub struct Stdout {
// FIXME: this should be LineWriter or BufWriter depending on the state of
// stdout (tty or not). Note that if this is not line buffered it
// should also flush-on-panic or some form of flush-on-abort.
inner: &'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>,
inner: Pin<&'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>>,
}

/// A locked reference to the `Stdout` handle.
Expand Down Expand Up @@ -550,25 +551,29 @@ pub struct StdoutLock<'a> {
pub fn stdout() -> Stdout {
static INSTANCE: SyncOnceCell<ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>> =
SyncOnceCell::new();

fn cleanup() {
if let Some(instance) = INSTANCE.get() {
// Flush the data and disable buffering during shutdown
// by replacing the line writer by one with zero
// buffering capacity.
// We use try_lock() instead of lock(), because someone
// might have leaked a StdoutLock, which would
// otherwise cause a deadlock here.
if let Some(lock) = Pin::static_ref(instance).try_lock() {
*lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
}
}
}

Stdout {
inner: INSTANCE.get_or_init(|| unsafe {
let _ = sys_common::at_exit(|| {
if let Some(instance) = INSTANCE.get() {
// Flush the data and disable buffering during shutdown
// by replacing the line writer by one with zero
// buffering capacity.
// We use try_lock() instead of lock(), because someone
// might have leaked a StdoutLock, which would
// otherwise cause a deadlock here.
if let Some(lock) = instance.try_lock() {
*lock.borrow_mut() = LineWriter::with_capacity(0, stdout_raw());
}
}
});
let r = ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())));
r.init();
r
}),
inner: Pin::static_ref(&INSTANCE).get_or_init_pin(
|| unsafe {
let _ = sys_common::at_exit(cleanup);
ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())))
},
|mutex| unsafe { mutex.init() },
),
}
}

Expand Down Expand Up @@ -700,7 +705,7 @@ impl fmt::Debug for StdoutLock<'_> {
/// an error.
#[stable(feature = "rust1", since = "1.0.0")]
pub struct Stderr {
inner: &'static ReentrantMutex<RefCell<StderrRaw>>,
inner: Pin<&'static ReentrantMutex<RefCell<StderrRaw>>>,
}

/// A locked reference to the `Stderr` handle.
Expand Down Expand Up @@ -762,11 +767,10 @@ pub fn stderr() -> Stderr {
static INSTANCE: SyncOnceCell<ReentrantMutex<RefCell<StderrRaw>>> = SyncOnceCell::new();

Stderr {
inner: INSTANCE.get_or_init(|| unsafe {
let r = ReentrantMutex::new(RefCell::new(stderr_raw()));
r.init();
r
}),
inner: Pin::static_ref(&INSTANCE).get_or_init_pin(
|| unsafe { ReentrantMutex::new(RefCell::new(stderr_raw())) },
|mutex| unsafe { mutex.init() },
),
}
}

Expand Down
2 changes: 2 additions & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@
#![feature(format_args_nl)]
#![feature(gen_future)]
#![feature(generator_trait)]
#![feature(get_mut_unchecked)]
#![feature(global_asm)]
#![feature(hashmap_internals)]
#![feature(int_error_internals)]
Expand Down Expand Up @@ -293,6 +294,7 @@
#![feature(panic_info_message)]
#![feature(panic_internals)]
#![feature(panic_unwind)]
#![feature(pin_static_ref)]
#![feature(prelude_import)]
#![feature(ptr_internals)]
#![feature(raw)]
Expand Down
52 changes: 19 additions & 33 deletions library/std/src/sys_common/remutex.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#[cfg(all(test, not(target_os = "emscripten")))]
mod tests;

use crate::fmt;
use crate::marker::PhantomPinned;
use crate::ops::Deref;
use crate::panic::{RefUnwindSafe, UnwindSafe};
use crate::pin::Pin;
use crate::sys::mutex as sys;

/// A re-entrant mutual exclusion
Expand All @@ -14,6 +15,7 @@ use crate::sys::mutex as sys;
pub struct ReentrantMutex<T> {
inner: sys::ReentrantMutex,
data: T,
_pinned: PhantomPinned,
}

unsafe impl<T: Send> Send for ReentrantMutex<T> {}
Expand All @@ -36,7 +38,7 @@ impl<T> RefUnwindSafe for ReentrantMutex<T> {}
/// guarded data.
#[must_use = "if unused the ReentrantMutex will immediately unlock"]
pub struct ReentrantMutexGuard<'a, T: 'a> {
lock: &'a ReentrantMutex<T>,
lock: Pin<&'a ReentrantMutex<T>>,
}

impl<T> !Send for ReentrantMutexGuard<'_, T> {}
Expand All @@ -50,7 +52,11 @@ impl<T> ReentrantMutex<T> {
/// once this mutex is in its final resting place, and only then are the
/// lock/unlock methods safe.
pub const unsafe fn new(t: T) -> ReentrantMutex<T> {
ReentrantMutex { inner: sys::ReentrantMutex::uninitialized(), data: t }
ReentrantMutex {
inner: sys::ReentrantMutex::uninitialized(),
data: t,
_pinned: PhantomPinned,
}
}

/// Initializes this mutex so it's ready for use.
Expand All @@ -59,8 +65,8 @@ impl<T> ReentrantMutex<T> {
///
/// Unsafe to call more than once, and must be called after this will no
/// longer move in memory.
pub unsafe fn init(&self) {
self.inner.init();
pub unsafe fn init(self: Pin<&mut Self>) {
self.get_unchecked_mut().inner.init()
}

/// Acquires a mutex, blocking the current thread until it is able to do so.
Expand All @@ -75,9 +81,9 @@ impl<T> ReentrantMutex<T> {
/// If another user of this mutex panicked while holding the mutex, then
/// this call will return failure if the mutex would otherwise be
/// acquired.
pub fn lock(&self) -> ReentrantMutexGuard<'_, T> {
pub fn lock(self: Pin<&Self>) -> ReentrantMutexGuard<'_, T> {
unsafe { self.inner.lock() }
ReentrantMutexGuard::new(&self)
ReentrantMutexGuard { lock: self }
}

/// Attempts to acquire this lock.
Expand All @@ -92,8 +98,12 @@ impl<T> ReentrantMutex<T> {
/// If another user of this mutex panicked while holding the mutex, then
/// this call will return failure if the mutex would otherwise be
/// acquired.
pub fn try_lock(&self) -> Option<ReentrantMutexGuard<'_, T>> {
if unsafe { self.inner.try_lock() } { Some(ReentrantMutexGuard::new(&self)) } else { None }
pub fn try_lock(self: Pin<&Self>) -> Option<ReentrantMutexGuard<'_, T>> {
if unsafe { self.inner.try_lock() } {
Some(ReentrantMutexGuard { lock: self })
} else {
None
}
}
}

Expand All @@ -106,30 +116,6 @@ impl<T> Drop for ReentrantMutex<T> {
}
}

impl<T: fmt::Debug + 'static> fmt::Debug for ReentrantMutex<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.try_lock() {
Some(guard) => f.debug_struct("ReentrantMutex").field("data", &*guard).finish(),
None => {
struct LockedPlaceholder;
impl fmt::Debug for LockedPlaceholder {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("<locked>")
}
}

f.debug_struct("ReentrantMutex").field("data", &LockedPlaceholder).finish()
}
}
}
}

impl<'mutex, T> ReentrantMutexGuard<'mutex, T> {
fn new(lock: &'mutex ReentrantMutex<T>) -> ReentrantMutexGuard<'mutex, T> {
ReentrantMutexGuard { lock }
}
}

impl<T> Deref for ReentrantMutexGuard<'_, T> {
type Target = T;

Expand Down
35 changes: 20 additions & 15 deletions library/std/src/sys_common/remutex/tests.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
use crate::boxed::Box;
use crate::cell::RefCell;
use crate::pin::Pin;
use crate::sync::Arc;
use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
use crate::thread;

#[test]
fn smoke() {
let m = unsafe {
let m = ReentrantMutex::new(());
m.init();
let mut m = Box::pin(ReentrantMutex::new(()));
m.as_mut().init();
m
};
let m = m.as_ref();
{
let a = m.lock();
{
Expand All @@ -27,18 +30,19 @@ fn smoke() {
#[test]
fn is_mutex() {
let m = unsafe {
let m = Arc::new(ReentrantMutex::new(RefCell::new(0)));
m.init();
m
// FIXME: Simplify this if Arc gets a Arc::get_pin_mut.
let mut m = Arc::new(ReentrantMutex::new(RefCell::new(0)));
Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init();
Pin::new_unchecked(m)
};
let m2 = m.clone();
let lock = m.lock();
let lock = m.as_ref().lock();
let child = thread::spawn(move || {
let lock = m2.lock();
let lock = m2.as_ref().lock();
assert_eq!(*lock.borrow(), 4950);
});
for i in 0..100 {
let lock = m.lock();
let lock = m.as_ref().lock();
*lock.borrow_mut() += i;
}
drop(lock);
Expand All @@ -48,20 +52,21 @@ fn is_mutex() {
#[test]
fn trylock_works() {
let m = unsafe {
let m = Arc::new(ReentrantMutex::new(()));
m.init();
m
// FIXME: Simplify this if Arc gets a Arc::get_pin_mut.
let mut m = Arc::new(ReentrantMutex::new(()));
Pin::new_unchecked(Arc::get_mut_unchecked(&mut m)).init();
Pin::new_unchecked(m)
};
let m2 = m.clone();
let _lock = m.try_lock();
let _lock2 = m.try_lock();
let _lock = m.as_ref().try_lock();
let _lock2 = m.as_ref().try_lock();
thread::spawn(move || {
let lock = m2.try_lock();
let lock = m2.as_ref().try_lock();
assert!(lock.is_none());
})
.join()
.unwrap();
let _lock3 = m.try_lock();
let _lock3 = m.as_ref().try_lock();
}

pub struct Answer<'a>(pub ReentrantMutexGuard<'a, RefCell<u32>>);
Expand Down

0 comments on commit 67c18fd

Please sign in to comment.