Replace `std::task` with `std::thread`, remove librustrt, make final "runtime" features lazily initialize #19654

aturon · Dec 9, 2014

This PR substantially narrows the notion of a "runtime" in Rust, and allows calling into Rust code directly without any setup or teardown.

After this PR, the basic "runtime support" in Rust will consist of:

Unwinding and backtrace support
Stack guards

Other support, such as helper threads for timers or the notion of a "current thread" are initialized automatically upon first use.

When using Rust in an embedded context, it should now be possible to call a Rust function directly as a C function with absolutely no setup, though in that case panics will cause the process to abort. In this regard, the C/Rust interface will look much like the C/C++ interface.

In more detail, this PR:

Merges librustrt back into std::rt, undoing the facade. While doing so, it removes a substantial amount of redundant functionality (such as mutexes defined in the rt module). Code using librustrt can now call into std::rt to e.g. start executing Rust code with unwinding support.
Allows all runtime data to be initialized lazily, including the "current thread", the "at_exit" infrastructure, and the "args" storage.
Deprecates and largely removes std::task along with the widespread requirement that there be a "current task" for many APIs in std. The entire task infrastructure is replaced with std::thread, which provides a more standard API for manipulating and creating native OS threads. In particular, it's possible to join on a created thread, and to get a handle to the currently-running thread. In addition, threads are equipped with some basic blocking support in the form of park/unpark operations (following a tradition in some OSes as well as the JVM). See the std::thread documentation for more details.
Channels are refactored to use a new internal blocking infrastructure that itself sits on top of park/unpark.

One important change here is that a Rust program ends when its main thread does, following most threading models. On the other hand, threads will often be created with an RAII-style join handle that will re-institute blocking semantics naturally (and with finer control).

This is very much a:

[breaking-change]

Closes #18000
r? @alexcrichton

aturon · Dec 9, 2014

Note: this PR does not attempt to exhaustively replace the task terminology with thread, nor does it clear out all use of the now-deprecated APIs. Both are left to follow-up work.

aturon · Dec 9, 2014

Note also: the implementation of park/unpark is currently pretty silly: it just uses a lock and condvar. In the future these can be implemented in more clever ways, e.g. following the JVM.

eddyb · Dec 9, 2014

cc @thestinger

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libcore/borrow.rs

@@ -92,7 +92,7 @@ impl<'a, T, Sized? B> BorrowFrom<Cow<'a, T, B>> for B where B: ToOwned<T> {

 /// Trait for moving into a `Cow`
 pub trait IntoCow<'a, T, Sized? B> {
-    /// Moves `self` into `Cow`
+    /// Moves `serlf` into `Cow`


alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/librustc_driver/lib.rs

@@ -473,18 +473,22 @@ pub fn monitor(f: proc():Send) {
    static STACK_SIZE: uint = 32000000; // 32MB

    let (tx, rx) = channel();
-    let w = io::ChanWriter::new(tx);
+    let mut w = Some(io::ChanWriter::new(tx)); // option dance


How come this dance is necessary? I would have thought that it could move into the proc below

If I am not mistaken, this is due to a limitation of closures not being able to capture w directly. I've seen this trick several places in the code-base, hoping it would disappear at some point - perhaps with unboxed closures?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/librustc_trans/back/write.rs

@@ -868,7 +868,11 @@ fn run_work_multithreaded(sess: &Session,
        let diag_emitter = diag_emitter.clone();
        let remark = sess.opts.cg.remark.clone();

-        let future = TaskBuilder::new().named(format!("codegen-{}", i)).try_future(proc() {
+        let (tx, rx) = channel();
+        let mut tx = Some(tx);


Similar to above, how come this needs an option?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/comm/blocking.rs

+pub struct WaitToken {
+    inner: Arc<Inner>,
+    no_send: NoSend,
+}


Should this also require NoSync to prevent from sharing amongst fork-join parallelism? I'm thinking of a function like this, but it may only be safe with Send + Sync anyway, so this is probably enough.

Right now the only safe bound for that function is Send + Sync, since Sync is overly permissive (allows stuff like thread-local references or other non-transferrable types).

aturon · Dec 9, 2014

Once piece of feedback I'd like: is spawn and with_join the right setup? That makes detached threads the "default". We could alternatively do spawn and spawn_detached, but I worry this will be surprising to people.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/comm/blocking.rs

+    no_send: NoSend,
+}
+
+pub fn tokens() -> (WaitToken, SignalToken) {


Out of curiosity, doesn't this abstraction break if the thread is blocked on anything other than WaitToken while some SignalTokens are signaled? I'm thinking that you call tokens, signal the token, manually call Thread::park(), and then call wait, you'd sleep forever, right? That seems pretty silly though, and I imagine we could document this example when it's a public-facing API.

It's correct that you won't get unparked by that signal, but you could be unparked by a different signal. But in any case, you're probably doing something wrong.

(Note that the intent of structures like this and park/unpark is for building blocking concurrency abstractions like channels; they aren't a good fit for application-level concurrent programming. See http://gee.cs.oswego.edu/dl/papers/aqs.pdf)

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/comm/sync.rs

-    /// The state field is protected by this mutex
-    lock: NativeMutex,
-    state: UnsafeCell<State<T>>,
+    lock: Mutex<State<T>>,


You know, I probably could've saved myself a lot of pain by also replacing the queue and buf data structures with something from std. Mutexes around Copy data ... bad news.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/comm/oneshot.rs

-                    _ => unreachable!()
-                }
-            });
+            let (wait_token, signal_token) = blocking::tokens();


I somewhat fear the perf impact of creating a new set of tokens each time a task blocks, but I'd personally rather land the runtime removal and optimize later.

Same here. FWIW, it would be quite easy to re-instate the BlockedTask-style enum.

Although note, this is basically just one allocation per context switch. Probably a drop in the bucket.

agreed, I'd want to measure before taking any action

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/io/stdio.rs

@@ -531,7 +522,7 @@ mod tests {
        assert_eq!(r.read_to_string().unwrap(), "hello!\n");
    }

-    #[test]
+    //#[test]


Nope: it's the reason this PR is titled [WIP].

aha! I see :)

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/lib.rs


 pub mod task;
+#[allow(missing_docs)]


Could this be removed now?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/args.rs


    static mut GLOBAL_ARGS_PTR: uint = 0;
-    static LOCK: StaticNativeMutex = NATIVE_MUTEX_INIT;
+    static LOCK: NativeMutex = MUTEX_INIT;


NativeMutex is gone, right?

As you can see, I develop on macos.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/at_exit_imp.rs

    }
 }

 pub fn push(f: proc():Send) {
+    INIT.doit(init);


alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/librustrt/at_exit_imp.rs

    unsafe {
        // Note that the check against 0 for the queue pointer is not atomic at
        // all with respect to `run`, meaning that this could theoretically be a
        // use-after-free. There's not much we can do to protect against that,
        // however. Let's just assume a well-behaved runtime and go from there!
-        rtassert!(!RUNNING.load(atomic::SeqCst));


Isn't this an important assertion to keep? If you re-enqueue some cleanup while we're cleaning up we'll just leak it?

Or... you'll hit the assertion below, nevermind!

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/mod.rs

+        args::init(argc, argv);
+        sys::thread::guard::init();
+        sys::stack_overflow::init();
+        unwind::register(failure::on_fail);


Do you think this could be moved to a Once?

The whole init function? It's an interesting idea...

Maybe not the whole function, but at least unwind::register. The others may be more difficult to move around.

I'm also fine developing our story here over time and not hammering it out all immediately.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/mod.rs

-///
-/// This function will only return once *all* native threads in the system have
-/// exited.
+// FIXME: this should be unsafe


Feel free to make it so :)

Also feel free to merge this upwards into lang_start

I tried to mark it unsafe and ran into a weird issue so left it alone. I can try it again.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/mod.rs

@@ -162,5 +195,7 @@ pub fn start(argc: int, argv: *const *const u8, main: proc()) -> int {
 /// Invoking cleanup while portions of the runtime are still in use may cause
 /// undefined behavior.
 pub unsafe fn cleanup() {
-    rustrt::cleanup();
+    args::cleanup();
+    sys::stack_overflow::cleanup();


Perhaps both args and stack_overflow could use at_exit to schedule cleanups? That way we only have to get one thing working with atexit to get it all to work.

sfackler · Dec 9, 2014

sfackler reviewed Dec 9, 2014

View changes

sfackler · Dec 9, 2014

src/libstd/sys/windows/thread.rs

+    if ret as uint == 0 {
+        // be sure to not leak the closure
+        let _p: Box<proc():Send> = mem::transmute(arg);
+        panic!("failed to spawn native thread: {}", ret);


Should there be an option to handle this error rather than panic?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/rt/util.rs

+// Indicates whether we should perform expensive sanity checks, including rtassert!
+//
+// FIXME: Once the runtime matures remove the `true` below to turn off rtassert,
+//        etc.


This comment makes me chuckle

sfackler · Dec 9, 2014

sfackler reviewed Dec 9, 2014

View changes

sfackler · Dec 9, 2014

src/libstd/sys/windows/stack_overflow.rs

+    }
+}
+
+impl Drop for Handler {


Does this need to impl Drop explicitly or can it just contain NoCopy?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/sys/common/thread_info.rs

+    thread: Thread,
+}
+
+thread_local!(static THREAD_INFO: RefCell<Option<ThreadInfo>> = RefCell::new(None))


Instead of an Option, perhaps this could have the lazy initialization expression here?

The lazy init expression is only used if you access the thread info from a thread not launched via Rust APIs; otherwise it is explicitly initialized with an already-created Thread.

Ah yeah after looking below I see that this is necessary, carry on!

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/sys/common/thread_info.rs

+
+impl ThreadInfo {
+    fn with<R>(f: |&mut ThreadInfo| -> R) -> R {
+        THREAD_INFO.with(|c| {


This borrow worries me a little in that once the THREAD_INFO has its destructor run, all blocking services will abort the process because they won't be able to access the current thread.

I'm not really sure of a great way to get around that, but perhaps this could yield a nicer panic message?

I can imagine a few ways to do better, but they involve bubbling up this possibility in APIs, e.g. by making Thread::current() yield an Option. I'm not sure how much to worry about this edge case.

I'm also not sure how to generate a better panic message here -- isn't with going to cause the panic? Looking at the docs, it doesn't seem like .destroyed covers all the cases that might lead to this panic.

For generating a better panic message you'd want to check .destroyed() beforehand yeah. I think that may be good enough for now, we could possibly add more support later for something like repeated re-construction if necessary.

sfackler · Dec 9, 2014

sfackler reviewed Dec 9, 2014

View changes

sfackler · Dec 9, 2014

src/libstd/rt/util.rs

+    let hash = msg.bytes().fold(0, |accum, val| accum + (val as uint) );
+    let quote = match hash % 10 {
+        0 => "
+It was from the artists and poets that the pertinent answers came, and I


Glad to see this is surviving the transition :)

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/thread.rs

+//! shared-memory data structures. In particular, types that are guaranteed to
+//! be threadsafe are easily shared between threads using the
+//! atomically-reference-counted container,
+//! [`Arc`](../../std/sync/struct.Arc.html).


This is somewhat oddly worded to me where it starts out by saying memory is isolated, but then it explains that I can indeed share memory. Maybe the emphasis at the top could be toned down a bit?

Sure. (This was brought over from the old std::task docs, so I was trying not to change it too much, but I agree with you.)

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/thread.rs

+    stdout: Option<Box<Writer + Send>>,
+    // Thread-local stderr
+    stderr: Option<Box<Writer + Send>>,
+}


The stdout and stderr options here seem pretty sketchy to me, I'd be fine removing them. They're implemented with a trivial wrapper around the provide proc regardless.

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/thread.rs

+
+/// Thread configuation. Provides detailed control over the properties
+/// and behavior of new threads.
+pub struct Cfg {


Cfg seems like a somewhat odd name for this, convention-wise it seems like it may want to be ThreadBuilder, but I do agree that's pretty long. We may be out of a noun like Command is to Process, but perhaps this could just be called Builder to avoid stuttering?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/thread.rs

+            // suffices. The second is that unwinding while unwinding is not
+            // defined.  We take care of that by having an 'unwinding' flag in
+            // the thread itself. For these reasons, this unsafety should be ok.
+            unsafe {


Can this unsafety be contained to just unwind::try?

alexcrichton · Dec 9, 2014

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton · Dec 9, 2014

src/libstd/thread.rs

+    }
+
+    /// Spawn a joinable thread, and return an RAII guard for it.
+    pub fn with_join<T: Send>(f: proc():Send -> T) -> JoinGuard<T> {


This is a bit of an interesting choice of words here! Perhaps we could move spawn to the constructor new (aka Thread::new) and free up the word spawn for this to give us some more room, but that's still not exactly the best state of affairs.

See #19654 (comment)

Could you also expand the documentation to explain where the type parameter T comes into play and how you can use a JoinGuard to get it out? It may also be nice to warn about ignoring the JoinGuard and how it may block.

alexcrichton · Dec 19, 2014

r+

p=10

bors · Dec 19, 2014

saw approval from alexcrichton
at aturon@903c5a8

merging aturon/rust/merge-rt = 903c5a8 into auto

status: {"merge_sha": "0efafac398ff7f28c5f0fe756c15b9008b3e0534"}

aturon/rust/merge-rt = 903c5a8 merged ok, testing candidate = 0efafac

fast-forwarding master to auto = 0efafac

bluss · Dec 19, 2014

spawn() and other adjacent features like channel() were in the prelude, saying something to the new Rust user about Rust the language in the process. I think it would be nice to preserve that, the feel of being setup up for concurrency by default.

reem · Dec 19, 2014

It also sort of suggested that they were builtin constructs, which I think their removal from the prelude is supposed to prevent.

aturon · Dec 19, 2014

@bluss You might want to leave a comment at rust-lang/rfcs#503, which talks about the future of the prelude.

aturon force-pushed the aturon:merge-rt branch from 5fca87f to 903c5a8 Dec 19, 2014

bors merged commit 903c5a8 into rust-lang:master Dec 19, 2014
2 checks passed

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details

default all tests passed

tomaka referenced this pull request Dec 19, 2014
Closed
rust-hl-modules fails with file operations? #15

This was referenced Dec 19, 2014

Closed

Revamp documentation to talk about `std::thread` #20038

Closed

Panicking tasks should abort process if not handled rust-lang/rfcs#519

zsiciarz referenced this pull request Dec 21, 2014
Closed
librustrt got removed #127

nukep referenced this pull request Dec 21, 2014
Merged
Remove librustrt, audio Task #242

ozkriff referenced this pull request Dec 22, 2014
Closed
Crashes on android after runtime removal #16

steveklabnik referenced this pull request Dec 22, 2014
Closed
std::rt docs are out of date #19442

mneumann referenced this pull request Jan 7, 2015
Closed
Panic: Duplicate mutex destroy on Thread.join (DragonFly) #20698

rust-lang/rust

Join GitHub today

Replace `std::task` with `std::thread`, remove librustrt, make final "runtime" features lazily initialize #19654

Conversation

aturon commented Dec 9, 2014

This comment has been minimized.

aturon commented Dec 9, 2014

This comment has been minimized.

aturon commented Dec 9, 2014

This comment has been minimized.

eddyb commented Dec 9, 2014

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aturon commented Dec 9, 2014

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

sfackler reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

sfackler reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sfackler reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014 View changes

This comment has been minimized.

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

sfackler reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

sfackler reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

sfackler reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

alexcrichton reviewed Dec 9, 2014

View changes

aturon force-pushed the aturon:merge-rt branch from `5fca87f` to `903c5a8` Dec 19, 2014

alexcrichton commented on `903c5a8` Dec 19, 2014

bors commented on `903c5a8` Dec 19, 2014