From 8334dd445fb10089a68808e7895f0c00d6fd0b3e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 4 Mar 2014 17:27:43 -0800 Subject: [PATCH 1/4] native: Stop using readdir() This function is not threadsafe, and is deprecated in favor of the threadsafe readdir_r variant. Closes #12692 --- src/libnative/io/file_unix.rs | 16 +++++++++++----- src/libstd/libc.rs | 9 ++++++--- src/rt/rust_builtin.c | 15 ++++++++++++--- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/libnative/io/file_unix.rs b/src/libnative/io/file_unix.rs index 4b6d1813ffa4a..cf9ada97a3254 100644 --- a/src/libnative/io/file_unix.rs +++ b/src/libnative/io/file_unix.rs @@ -19,6 +19,7 @@ use std::libc; use std::mem; use std::rt::rtio; use std::vec; +use std::vec_ng::Vec; use io::{IoResult, retry, keep_going}; @@ -341,7 +342,7 @@ pub fn mkdir(p: &CString, mode: io::FilePermission) -> IoResult<()> { pub fn readdir(p: &CString) -> IoResult<~[Path]> { use std::libc::{dirent_t}; - use std::libc::{opendir, readdir, closedir}; + use std::libc::{opendir, readdir_r, closedir}; fn prune(root: &CString, dirs: ~[Path]) -> ~[Path] { let root = unsafe { CString::new(root.with_ref(|p| p), false) }; @@ -353,9 +354,14 @@ pub fn readdir(p: &CString) -> IoResult<~[Path]> { } extern { - fn rust_list_dir_val(ptr: *dirent_t) -> *libc::c_char; + fn rust_dirent_t_size() -> libc::c_int; + fn rust_list_dir_val(ptr: *mut dirent_t) -> *libc::c_char; } + let size = unsafe { rust_dirent_t_size() }; + let mut buf = Vec::::with_capacity(size as uint); + let ptr = buf.as_mut_slice().as_mut_ptr() as *mut dirent_t; + debug!("os::list_dir -- BEFORE OPENDIR"); let dir_ptr = p.with_ref(|buf| unsafe { opendir(buf) }); @@ -363,13 +369,13 @@ pub fn readdir(p: &CString) -> IoResult<~[Path]> { if dir_ptr as uint != 0 { let mut paths = ~[]; debug!("os::list_dir -- opendir() SUCCESS"); - let mut entry_ptr = unsafe { readdir(dir_ptr) }; - while entry_ptr as uint != 0 { + let mut entry_ptr = 0 as *mut dirent_t; + while unsafe { readdir_r(dir_ptr, ptr, &mut entry_ptr) == 0 } { + if entry_ptr.is_null() { break } let cstr = unsafe { CString::new(rust_list_dir_val(entry_ptr), false) }; paths.push(Path::new(cstr)); - entry_ptr = unsafe { readdir(dir_ptr) }; } assert_eq!(unsafe { closedir(dir_ptr) }, 0); Ok(prune(p, paths)) diff --git a/src/libstd/libc.rs b/src/libstd/libc.rs index ef641bbb6657f..7096765d9fc3b 100644 --- a/src/libstd/libc.rs +++ b/src/libstd/libc.rs @@ -3657,13 +3657,16 @@ pub mod funcs { pub unsafe fn opendir(dirname: *c_char) -> *DIR { rust_opendir(dirname) } - pub unsafe fn readdir(dirp: *DIR) -> *dirent_t { - rust_readdir(dirp) + pub unsafe fn readdir_r(dirp: *DIR, + entry: *mut dirent_t, + result: *mut *mut dirent_t) -> c_int { + rust_readdir_r(dirp, entry, result) } extern { fn rust_opendir(dirname: *c_char) -> *DIR; - fn rust_readdir(dirp: *DIR) -> *dirent_t; + fn rust_readdir_r(dirp: *DIR, entry: *mut dirent_t, + result: *mut *mut dirent_t) -> c_int; } extern { diff --git a/src/rt/rust_builtin.c b/src/rt/rust_builtin.c index 81eba2984dad0..6ba121c5d2d04 100644 --- a/src/rt/rust_builtin.c +++ b/src/rt/rust_builtin.c @@ -279,9 +279,14 @@ rust_opendir(char *dirname) { return opendir(dirname); } -struct dirent* -rust_readdir(DIR *dirp) { - return readdir(dirp); +int +rust_readdir_r(DIR *dirp, struct dirent *entry, struct dirent **result) { + return readdir_r(dirp, entry, result); +} + +int +rust_dirent_t_size() { + return sizeof(struct dirent); } #else @@ -294,6 +299,10 @@ void rust_readdir() { } +void +rust_dirent_t_size() { +} + #endif uintptr_t From d8bd8de82e19702ad26fff704ff9a4890ebe1bf7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 4 Mar 2014 17:40:45 -0800 Subject: [PATCH 2/4] native: Move from usleep() to nanosleep() Using nanosleep() allows us to gracefully recover from EINTR because on error it fills in the second parameter with the remaining time to sleep. Closes #12689 --- src/libnative/io/timer_other.rs | 11 +++++++++-- src/libnative/io/timer_timerfd.rs | 15 +++++++++++---- src/libstd/libc.rs | 2 ++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/libnative/io/timer_other.rs b/src/libnative/io/timer_other.rs index 9d70055086359..d7323ddf49955 100644 --- a/src/libnative/io/timer_other.rs +++ b/src/libnative/io/timer_other.rs @@ -218,8 +218,15 @@ impl Timer { } pub fn sleep(ms: u64) { - // FIXME: this can fail because of EINTR, what do do? - let _ = unsafe { libc::usleep((ms * 1000) as libc::c_uint) }; + let mut to_sleep = libc::timespec { + tv_sec: (ms / 1000) as libc::time_t, + tv_nsec: ((ms % 1000) * 1000000) as libc::c_long, + }; + while unsafe { libc::nanosleep(&to_sleep, &mut to_sleep) } != 0 { + if os::errno() as int != libc::EINTR as int { + fail!("failed to sleep, but not because of EINTR?"); + } + } } fn inner(&mut self) -> ~Inner { diff --git a/src/libnative/io/timer_timerfd.rs b/src/libnative/io/timer_timerfd.rs index 68277efc9b714..55301b6f7c8d6 100644 --- a/src/libnative/io/timer_timerfd.rs +++ b/src/libnative/io/timer_timerfd.rs @@ -23,8 +23,8 @@ //! why). //! //! As with timer_other, timers just using sleep() do not use the timerfd at -//! all. They remove the timerfd from the worker thread and then invoke usleep() -//! to block the calling thread. +//! all. They remove the timerfd from the worker thread and then invoke +//! nanosleep() to block the calling thread. //! //! As with timer_other, all units in this file are in units of millseconds. @@ -183,8 +183,15 @@ impl Timer { } pub fn sleep(ms: u64) { - // FIXME: this can fail because of EINTR, what do do? - let _ = unsafe { libc::usleep((ms * 1000) as libc::c_uint) }; + let mut to_sleep = libc::timespec { + tv_sec: (ms / 1000) as libc::time_t, + tv_nsec: ((ms % 1000) * 1000000) as libc::c_long, + }; + while unsafe { libc::nanosleep(&to_sleep, &mut to_sleep) } != 0 { + if os::errno() as int != libc::EINTR as int { + fail!("failed to sleep, but not because of EINTR?"); + } + } } fn remove(&mut self) { diff --git a/src/libstd/libc.rs b/src/libstd/libc.rs index 7096765d9fc3b..d10fa7684e83c 100644 --- a/src/libstd/libc.rs +++ b/src/libstd/libc.rs @@ -3682,6 +3682,7 @@ pub mod funcs { use libc::types::common::c95::c_void; use libc::types::os::arch::c95::{c_char, c_int, c_long, c_uint}; use libc::types::os::arch::c95::{size_t}; + use libc::types::os::common::posix01::timespec; use libc::types::os::arch::posix01::utimbuf; use libc::types::os::arch::posix88::{gid_t, off_t, pid_t}; use libc::types::os::arch::posix88::{ssize_t, uid_t}; @@ -3731,6 +3732,7 @@ pub mod funcs { pub fn setuid(uid: uid_t) -> c_int; pub fn sleep(secs: c_uint) -> c_uint; pub fn usleep(secs: c_uint) -> c_int; + pub fn nanosleep(rqtp: *timespec, rmtp: *mut timespec) -> c_int; pub fn sysconf(name: c_int) -> c_long; pub fn tcgetpgrp(fd: c_int) -> pid_t; pub fn ttyname(fd: c_int) -> *c_char; From e6acff828787b9b6c65ef66942f45e58b2f22ad6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 4 Mar 2014 18:20:46 -0800 Subject: [PATCH 3/4] native: Fix usage of a deallocated mutex When the timer_helper thread exited, it would attempt to re-acquire the global task count mutex, but the mutex had previously been deallocated, leading to undefined behavior of the mutex, and in some cases deadlock. Another mutex is used to coordinate shutting down the timer helper thread. Closes #12699 --- src/libnative/bookkeeping.rs | 9 +++------ src/libnative/io/timer_helper.rs | 21 +++++++++++---------- src/test/run-pass/issue-12699.rs | 22 ++++++++++++++++++++++ 3 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 src/test/run-pass/issue-12699.rs diff --git a/src/libnative/bookkeeping.rs b/src/libnative/bookkeeping.rs index b1addc5cda531..76e58ce753f21 100644 --- a/src/libnative/bookkeeping.rs +++ b/src/libnative/bookkeeping.rs @@ -39,12 +39,9 @@ pub fn decrement() { /// the entry points of native programs pub fn wait_for_other_tasks() { unsafe { - { - let mut guard = TASK_LOCK.lock(); - while TASK_COUNT.load(atomics::SeqCst) > 0 { - guard.wait(); - } + let mut guard = TASK_LOCK.lock(); + while TASK_COUNT.load(atomics::SeqCst) > 0 { + guard.wait(); } - TASK_LOCK.destroy(); } } diff --git a/src/libnative/io/timer_helper.rs b/src/libnative/io/timer_helper.rs index 7669d4a658fad..45bd0f8b67cf5 100644 --- a/src/libnative/io/timer_helper.rs +++ b/src/libnative/io/timer_helper.rs @@ -36,6 +36,8 @@ use task; static mut HELPER_CHAN: *mut Chan = 0 as *mut Chan; static mut HELPER_SIGNAL: imp::signal = 0 as imp::signal; +static mut TIMER_HELPER_EXIT: StaticNativeMutex = NATIVE_MUTEX_INIT; + pub fn boot(helper: fn(imp::signal, Port)) { static mut LOCK: StaticNativeMutex = NATIVE_MUTEX_INIT; static mut INITIALIZED: bool = false; @@ -53,6 +55,7 @@ pub fn boot(helper: fn(imp::signal, Port)) { task::spawn(proc() { bookkeeping::decrement(); helper(receive, msgp); + TIMER_HELPER_EXIT.lock().signal() }); rt::at_exit(proc() { shutdown() }); @@ -70,17 +73,15 @@ pub fn send(req: Req) { } fn shutdown() { - // We want to wait for the entire helper task to exit, and in doing so it - // will attempt to decrement the global task count. When the helper was - // created, it decremented the count so it wouldn't count towards preventing - // the program to exit, so here we pair that manual decrement with a manual - // increment. We will then wait for the helper thread to exit by calling - // wait_for_other_tasks. - bookkeeping::increment(); - // Request a shutdown, and then wait for the task to exit - send(Shutdown); - bookkeeping::wait_for_other_tasks(); + unsafe { + let mut guard = TIMER_HELPER_EXIT.lock(); + send(Shutdown); + guard.wait(); + drop(guard); + TIMER_HELPER_EXIT.destroy(); + } + // Clean up after ther helper thread unsafe { diff --git a/src/test/run-pass/issue-12699.rs b/src/test/run-pass/issue-12699.rs new file mode 100644 index 0000000000000..24f9a05b2c16d --- /dev/null +++ b/src/test/run-pass/issue-12699.rs @@ -0,0 +1,22 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +extern crate native; + +use std::io::timer; + +#[start] +fn start(argc: int, argv: **u8) -> int { + native::start(argc, argv, main) +} + +fn main() { + timer::sleep(250); +} From 9668ab58f338b27d8f53357c7fd4ea3d3f06305e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 5 Mar 2014 10:35:30 -0800 Subject: [PATCH 4/4] std: Move libnative task count bookkeeping to std When using tasks in Rust, the expectation is that the runtime does not exit before all tasks have exited. This is enforced in libgreen through the `SchedPool` type, and it is enforced in libnative through a `bookkeeping` module and a global count/mutex pair. Unfortunately, this means that a process which originates with libgreen will not wait for spawned native tasks. In order to fix this problem, the bookkeeping module was moved from libnative to libstd so the runtime itself can wait for native tasks to exit. Green tasks do not manage themselves through this bookkeeping module, but native tasks will continue to manage themselves through this module. Closes #12684 --- src/libnative/io/timer_helper.rs | 2 +- src/libnative/lib.rs | 2 -- src/libnative/task.rs | 4 +-- src/{libnative => libstd/rt}/bookkeeping.rs | 19 ++++++++------ src/libstd/rt/mod.rs | 4 +++ src/test/run-pass/issue-12684.rs | 28 +++++++++++++++++++++ src/test/run-pass/issue-12699.rs | 2 ++ 7 files changed, 49 insertions(+), 12 deletions(-) rename src/{libnative => libstd/rt}/bookkeeping.rs (73%) create mode 100644 src/test/run-pass/issue-12684.rs diff --git a/src/libnative/io/timer_helper.rs b/src/libnative/io/timer_helper.rs index 45bd0f8b67cf5..62e41771423d4 100644 --- a/src/libnative/io/timer_helper.rs +++ b/src/libnative/io/timer_helper.rs @@ -21,10 +21,10 @@ //! time. use std::cast; +use std::rt::bookkeeping; use std::rt; use std::unstable::mutex::{StaticNativeMutex, NATIVE_MUTEX_INIT}; -use bookkeeping; use io::timer::{Req, Shutdown}; use task; diff --git a/src/libnative/lib.rs b/src/libnative/lib.rs index 378283973dbe5..4b6942a108316 100644 --- a/src/libnative/lib.rs +++ b/src/libnative/lib.rs @@ -58,7 +58,6 @@ use std::os; use std::rt; -mod bookkeeping; pub mod io; pub mod task; @@ -105,6 +104,5 @@ pub fn start(argc: int, argv: **u8, main: proc()) -> int { /// number of arguments. pub fn run(main: proc()) -> int { main(); - bookkeeping::wait_for_other_tasks(); os::get_exit_status() } diff --git a/src/libnative/task.rs b/src/libnative/task.rs index aa3afd4c1a5f3..793e4d48e13de 100644 --- a/src/libnative/task.rs +++ b/src/libnative/task.rs @@ -16,19 +16,19 @@ use std::any::Any; use std::cast; +use std::rt::bookkeeping; use std::rt::env; use std::rt::local::Local; use std::rt::rtio; +use std::rt::stack; use std::rt::task::{Task, BlockedTask, SendMessage}; use std::rt::thread::Thread; use std::rt; use std::task::TaskOpts; use std::unstable::mutex::NativeMutex; -use std::rt::stack; use io; use task; -use bookkeeping; /// Creates a new Task which is ready to execute as a 1:1 task. pub fn new(stack_bounds: (uint, uint)) -> ~Task { diff --git a/src/libnative/bookkeeping.rs b/src/libstd/rt/bookkeeping.rs similarity index 73% rename from src/libnative/bookkeeping.rs rename to src/libstd/rt/bookkeeping.rs index 76e58ce753f21..5851a6a39c640 100644 --- a/src/libnative/bookkeeping.rs +++ b/src/libstd/rt/bookkeeping.rs @@ -8,16 +8,21 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//! 1:1 Task bookkeeping +//! Task bookkeeping //! -//! This module keeps track of the number of running 1:1 tasks so that entry -//! points with libnative know when it's possible to exit the program (once all -//! tasks have exited). +//! This module keeps track of the number of running tasks so that entry points +//! with libnative know when it's possible to exit the program (once all tasks +//! have exited). //! -//! The green counterpart for this is bookkeeping on sched pools. +//! The green counterpart for this is bookkeeping on sched pools, and it's up to +//! each respective runtime to make sure that they call increment() and +//! decrement() manually. -use std::sync::atomics; -use std::unstable::mutex::{StaticNativeMutex, NATIVE_MUTEX_INIT}; +#[experimental]; // this is a massive code smell +#[doc(hidden)]; + +use sync::atomics; +use unstable::mutex::{StaticNativeMutex, NATIVE_MUTEX_INIT}; static mut TASK_COUNT: atomics::AtomicUint = atomics::INIT_ATOMIC_UINT; static mut TASK_LOCK: StaticNativeMutex = NATIVE_MUTEX_INIT; diff --git a/src/libstd/rt/mod.rs b/src/libstd/rt/mod.rs index 2f1a698909298..459bc061c569d 100644 --- a/src/libstd/rt/mod.rs +++ b/src/libstd/rt/mod.rs @@ -128,6 +128,9 @@ pub mod args; // Support for running procedures when a program has exited. mod at_exit_imp; +// Bookkeeping for task counts +pub mod bookkeeping; + // Stack overflow protection pub mod stack; @@ -207,6 +210,7 @@ pub fn at_exit(f: proc()) { /// Invoking cleanup while portions of the runtime are still in use may cause /// undefined behavior. pub unsafe fn cleanup() { + bookkeeping::wait_for_other_tasks(); at_exit_imp::run(); args::cleanup(); local_ptr::cleanup(); diff --git a/src/test/run-pass/issue-12684.rs b/src/test/run-pass/issue-12684.rs new file mode 100644 index 0000000000000..a6eef4e20215f --- /dev/null +++ b/src/test/run-pass/issue-12684.rs @@ -0,0 +1,28 @@ +// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// ignore-fast + +extern crate native; +extern crate green; +extern crate rustuv; + +#[start] +fn start(argc: int, argv: **u8) -> int { green::start(argc, argv, main) } + +fn main() { + native::task::spawn(proc() customtask()); +} + +fn customtask() { + let mut timer = std::io::timer::Timer::new().unwrap(); + let periodic = timer.periodic(10); + periodic.recv(); +} diff --git a/src/test/run-pass/issue-12699.rs b/src/test/run-pass/issue-12699.rs index 24f9a05b2c16d..6409ba5137584 100644 --- a/src/test/run-pass/issue-12699.rs +++ b/src/test/run-pass/issue-12699.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// ignore-fast + extern crate native; use std::io::timer;