From bab15f773afd5724023d9a065b5af276e2468ff5 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 24 Sep 2020 17:45:50 +0200 Subject: [PATCH 1/5] Remove std::io::lazy::Lazy in favour of SyncOnceCell The (internal) std::io::lazy::Lazy was used to lazily initialize the stdout and stdin buffers (and mutexes). It uses atexit() to register a destructor to flush the streams on exit, and mark the streams as 'closed'. Using the stream afterwards would result in a panic. Stdout uses a LineWriter which contains a BufWriter that will flush the buffer on drop. This one is important to be executed during shutdown, to make sure no buffered output is lost. It also forbids access to stdout afterwards, since the buffer is already flushed and gone. Stdin uses a BufReader, which does not implement Drop. It simply forgets any previously read data that was not read from the buffer yet. This means that in the case of stdin, the atexit() function's only effect is making stdin inaccessible to the program, such that later accesses result in a panic. This is uncessary, as it'd have been safe to access stdin during shutdown of the program. --- This change removes the entire io::lazy module in favour of SyncOnceCell. SyncOnceCell's fast path is much faster (a single atomic operation) than locking a sys_common::Mutex on every access like Lazy did. However, SyncOnceCell does not use atexit() to drop the contained object during shutdown. As noted above, this is not a problem for stdin. It simply means stdin is now usable during shutdown. The atexit() call for stdout is moved to the stdio module. Unlike the now-removed Lazy struct, SyncOnceCell does not have a 'gone and unusable' state that panics. Instead of adding this again, this simply replaces the buffer with one with zero capacity. This effectively flushes the old buffer *and* makes any writes afterwards pass through directly without touching a buffer, making print!() available during shutdown without panicking. --- library/std/src/io/lazy.rs | 63 ------------------------------- library/std/src/io/mod.rs | 1 - library/std/src/io/stdio.rs | 74 ++++++++++++++++++++----------------- 3 files changed, 40 insertions(+), 98 deletions(-) delete mode 100644 library/std/src/io/lazy.rs diff --git a/library/std/src/io/lazy.rs b/library/std/src/io/lazy.rs deleted file mode 100644 index 1968d498bbed4..0000000000000 --- a/library/std/src/io/lazy.rs +++ /dev/null @@ -1,63 +0,0 @@ -use crate::cell::Cell; -use crate::ptr; -use crate::sync::Arc; -use crate::sys_common; -use crate::sys_common::mutex::Mutex; - -pub struct Lazy { - // We never call `lock.init()`, so it is UB to attempt to acquire this mutex reentrantly! - lock: Mutex, - ptr: Cell<*mut Arc>, -} - -#[inline] -const fn done() -> *mut Arc { - 1_usize as *mut _ -} - -unsafe impl Sync for Lazy {} - -impl Lazy { - pub const fn new() -> Lazy { - Lazy { lock: Mutex::new(), ptr: Cell::new(ptr::null_mut()) } - } -} - -impl Lazy { - /// Safety: `init` must not call `get` on the variable that is being - /// initialized. - pub unsafe fn get(&'static self, init: fn() -> Arc) -> Option> { - let _guard = self.lock.lock(); - let ptr = self.ptr.get(); - if ptr.is_null() { - Some(self.init(init)) - } else if ptr == done() { - None - } else { - Some((*ptr).clone()) - } - } - - // Must only be called with `lock` held - unsafe fn init(&'static self, init: fn() -> Arc) -> Arc { - // If we successfully register an at exit handler, then we cache the - // `Arc` allocation in our own internal box (it will get deallocated by - // the at exit handler). Otherwise we just return the freshly allocated - // `Arc`. - let registered = sys_common::at_exit(move || { - let ptr = { - let _guard = self.lock.lock(); - self.ptr.replace(done()) - }; - drop(Box::from_raw(ptr)) - }); - // This could reentrantly call `init` again, which is a problem - // because our `lock` allows reentrancy! - // That's why `get` is unsafe and requires the caller to ensure no reentrancy happens. - let ret = init(); - if registered.is_ok() { - self.ptr.set(Box::into_raw(Box::new(ret.clone()))); - } - ret - } -} diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index adea8a804e3ca..d9d0380781925 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -285,7 +285,6 @@ mod buffered; mod cursor; mod error; mod impls; -mod lazy; pub mod prelude; mod stdio; mod util; diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index 9974b65f1e164..e0e5ccd062533 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -7,10 +7,11 @@ use crate::io::prelude::*; use crate::cell::RefCell; use crate::fmt; -use crate::io::lazy::Lazy; use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter}; -use crate::sync::{Arc, Mutex, MutexGuard, Once}; +use crate::lazy::SyncOnceCell; +use crate::sync::{Arc, Mutex, MutexGuard}; use crate::sys::stdio; +use crate::sys_common; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; use crate::thread::LocalKey; @@ -292,15 +293,13 @@ pub struct StdinLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stdin() -> Stdin { - static INSTANCE: Lazy>> = Lazy::new(); - return Stdin { - inner: unsafe { INSTANCE.get(stdin_init).expect("cannot access stdin during shutdown") }, - }; - - fn stdin_init() -> Arc>> { - // This must not reentrantly access `INSTANCE` - let stdin = stdin_raw(); - Arc::new(Mutex::new(BufReader::with_capacity(stdio::STDIN_BUF_SIZE, stdin))) + static INSTANCE: SyncOnceCell>>> = SyncOnceCell::new(); + Stdin { + inner: INSTANCE + .get_or_init(|| { + Arc::new(Mutex::new(BufReader::with_capacity(stdio::STDIN_BUF_SIZE, stdin_raw()))) + }) + .clone(), } } @@ -534,19 +533,27 @@ pub struct StdoutLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stdout() -> Stdout { - static INSTANCE: Lazy>>> = Lazy::new(); - return Stdout { - inner: unsafe { INSTANCE.get(stdout_init).expect("cannot access stdout during shutdown") }, - }; - - fn stdout_init() -> Arc>>> { - // This must not reentrantly access `INSTANCE` - let stdout = stdout_raw(); - unsafe { - let ret = Arc::new(ReentrantMutex::new(RefCell::new(LineWriter::new(stdout)))); - ret.init(); - ret - } + static INSTANCE: SyncOnceCell>>>> = + SyncOnceCell::new(); + 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()); + } + } + }); + Arc::new(ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())))) + }) + .clone(), } } @@ -714,16 +721,15 @@ pub fn stderr() -> Stderr { // // This has the added benefit of allowing `stderr` to be usable during // process shutdown as well! - static INSTANCE: ReentrantMutex> = - unsafe { ReentrantMutex::new(RefCell::new(stderr_raw())) }; - - // When accessing stderr we need one-time initialization of the reentrant - // mutex. Afterwards we can just always use the now-filled-in `INSTANCE` value. - static INIT: Once = Once::new(); - INIT.call_once(|| unsafe { - INSTANCE.init(); - }); - Stderr { inner: &INSTANCE } + static INSTANCE: SyncOnceCell>> = SyncOnceCell::new(); + + Stderr { + inner: INSTANCE.get_or_init(|| unsafe { + let r = ReentrantMutex::new(RefCell::new(stderr_raw())); + r.init(); + r + }), + } } impl Stderr { From e9b25f520bd2e3687213aa1162e631b08b9bf7ed Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 24 Sep 2020 18:10:26 +0200 Subject: [PATCH 2/5] Add test to check stdout flushing during shutdown. --- src/test/ui/stdout-during-shutdown.rs | 14 ++++++++++++++ src/test/ui/stdout-during-shutdown.run.stdout | 1 + 2 files changed, 15 insertions(+) create mode 100644 src/test/ui/stdout-during-shutdown.rs create mode 100644 src/test/ui/stdout-during-shutdown.run.stdout diff --git a/src/test/ui/stdout-during-shutdown.rs b/src/test/ui/stdout-during-shutdown.rs new file mode 100644 index 0000000000000..c785fc0869610 --- /dev/null +++ b/src/test/ui/stdout-during-shutdown.rs @@ -0,0 +1,14 @@ +// run-pass +// check-run-results + +#![feature(rustc_private)] + +extern crate libc; + +fn main() { + extern "C" fn bye() { + print!(", world!"); + } + unsafe { libc::atexit(bye) }; + print!("hello"); +} diff --git a/src/test/ui/stdout-during-shutdown.run.stdout b/src/test/ui/stdout-during-shutdown.run.stdout new file mode 100644 index 0000000000000..30f51a3fba527 --- /dev/null +++ b/src/test/ui/stdout-during-shutdown.run.stdout @@ -0,0 +1 @@ +hello, world! \ No newline at end of file From 45700a9d58679131a628ddc95dd8ce0fcf1f2430 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 24 Sep 2020 19:09:33 +0200 Subject: [PATCH 3/5] Drop use of Arc from Stdin and Stdout. --- library/std/src/io/stdio.rs | 50 +++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index e0e5ccd062533..05bdbe0f563c6 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -9,7 +9,7 @@ use crate::cell::RefCell; use crate::fmt; use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter}; use crate::lazy::SyncOnceCell; -use crate::sync::{Arc, Mutex, MutexGuard}; +use crate::sync::{Mutex, MutexGuard}; use crate::sys::stdio; use crate::sys_common; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; @@ -218,7 +218,7 @@ fn handle_ebadf(r: io::Result, default: T) -> io::Result { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub struct Stdin { - inner: Arc>>, + inner: &'static Mutex>, } /// A locked reference to the `Stdin` handle. @@ -293,13 +293,11 @@ pub struct StdinLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stdin() -> Stdin { - static INSTANCE: SyncOnceCell>>> = SyncOnceCell::new(); + static INSTANCE: SyncOnceCell>> = SyncOnceCell::new(); Stdin { - inner: INSTANCE - .get_or_init(|| { - Arc::new(Mutex::new(BufReader::with_capacity(stdio::STDIN_BUF_SIZE, stdin_raw()))) - }) - .clone(), + inner: INSTANCE.get_or_init(|| { + Mutex::new(BufReader::with_capacity(stdio::STDIN_BUF_SIZE, stdin_raw())) + }), } } @@ -475,7 +473,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: Arc>>>, + inner: &'static ReentrantMutex>>, } /// A locked reference to the `Stdout` handle. @@ -533,27 +531,25 @@ pub struct StdoutLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stdout() -> Stdout { - static INSTANCE: SyncOnceCell>>>> = + static INSTANCE: SyncOnceCell>>> = SyncOnceCell::new(); 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()); - } + 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()); } - }); - Arc::new(ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw())))) - }) - .clone(), + } + }); + ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))) + }), } } From 6f9c1323a7a0fe162a5642e229d54afec7ccb299 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 24 Sep 2020 19:25:21 +0200 Subject: [PATCH 4/5] Call ReentrantMutex::init() in stdout(). --- library/std/src/io/stdio.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index 05bdbe0f563c6..b7d3c47e24b09 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -548,7 +548,9 @@ pub fn stdout() -> Stdout { } } }); - ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))) + let r = ReentrantMutex::new(RefCell::new(LineWriter::new(stdout_raw()))); + r.init(); + r }), } } From 6b8b9c4adbd7d8d53d730a3c01d154dec3bd179f Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 26 Sep 2020 11:47:26 +0200 Subject: [PATCH 5/5] Disable stdout-during-shutdown test on emscripten. --- src/test/ui/stdout-during-shutdown.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/ui/stdout-during-shutdown.rs b/src/test/ui/stdout-during-shutdown.rs index c785fc0869610..a6cf812ca644e 100644 --- a/src/test/ui/stdout-during-shutdown.rs +++ b/src/test/ui/stdout-during-shutdown.rs @@ -1,5 +1,10 @@ // run-pass // check-run-results +// ignore-emscripten + +// Emscripten doesn't flush its own stdout buffers on exit, which would fail +// this test. So this test is disabled on this platform. +// See https://emscripten.org/docs/getting_started/FAQ.html#what-does-exiting-the-runtime-mean-why-don-t-atexit-s-run #![feature(rustc_private)]