Tasks should not hold a ref to their parent (Close #1789)

Author: bblum

src/libcore/sys.rs

@@ -19,7 +19,6 @@ type rust_cond_lock = *libc::c_void;
 
 #[abi = "cdecl"]
 extern mod rustrt {
-    fn unsupervise();
     pure fn shape_log_str(t: *sys::type_desc, data: *()) -> ~str;
 
     fn rust_create_cond_lock() -> rust_cond_lock;

src/libcore/task.rs

@@ -731,8 +731,7 @@ fn spawn_raw(opts: task_opts, +f: fn~()) {
             // Getting killed after here would leak the task.
 
             let child_wrapper =
-                make_child_wrapper(new_task, child_tg,
-                                   opts.supervise, is_main, f);
+                make_child_wrapper(new_task, child_tg, is_main, f);
             let fptr = ptr::addr_of(child_wrapper);
             let closure: *rust_closure = unsafe::reinterpret_cast(fptr);
 
@@ -750,19 +749,14 @@ fn spawn_raw(opts: task_opts, +f: fn~()) {
     }
 
     fn make_child_wrapper(child_task: *rust_task, -child_tg: taskgroup_arc,
-                          supervise: bool, is_main: bool,
-                          -f: fn~()) -> fn~() {
+                          is_main: bool, -f: fn~()) -> fn~() {
         let child_tg_ptr = ~mut some(child_tg);
         fn~() {
             // Agh. Get move-mode items into the closure. FIXME (#2829)
             let mut child_tg_opt = none;
             *child_tg_ptr <-> child_tg_opt;
             let child_tg = option::unwrap(child_tg_opt);
             // Child task runs this code.
-            if !supervise {
-                // FIXME (#1868, #1789) take this out later
-                rustrt::unsupervise();
-            }
             // Set up membership in taskgroup. If this returns none, the
             // parent was already failing, so don't bother doing anything.
             alt enlist_in_taskgroup(child_tg, child_task) {
@@ -1018,7 +1012,6 @@ extern mod rustrt {
     fn start_task(task: *rust_task, closure: *rust_closure);
 
     fn rust_task_is_unwinding(task: *rust_task) -> bool;
-    fn unsupervise();
     fn rust_osmain_sched_id() -> sched_id;
     fn rust_task_inhibit_kill();
     fn rust_task_allow_kill();
@@ -1464,6 +1457,21 @@ fn test_unkillable_nested() {
     po.recv();
 }
 
+#[test]
+fn test_child_doesnt_ref_parent() {
+    // If the child refcounts the parent task, this will stack overflow when
+    // climbing the task tree to dereference each ancestor. (See #1789)
+    const generations: uint = 8192;
+    fn child_no(x: uint) -> fn~() {
+        ret || {
+            if x < generations {
+                task::spawn(child_no(x+1));
+            }
+        }
+    }
+    task::spawn(child_no(0));
+}
+
 #[test]
 fn test_tls_multitask() unsafe {
     fn my_key(+_x: @~str) { }

src/rt/rust_builtin.cpp

@@ -129,8 +129,7 @@ rust_env_pairs() {
 
 extern "C" CDECL void
 unsupervise() {
-    rust_task *task = rust_get_current_task();
-    task->unsupervise();
+    // FIXME(#1789): bblum: remove this; requires a snapshot
 }
 
 extern "C" CDECL void

src/rt/rust_sched_loop.cpp

@@ -76,9 +76,6 @@ rust_sched_loop::kill_all_tasks() {
     while (!all_tasks.empty()) {
         rust_task *task = all_tasks.back();
         all_tasks.pop_back();
-        // We don't want the failure of these tasks to propagate back
-        // to the kernel again since we're already failing everything
-        task->unsupervise();
         task->kill();
     }
 }
@@ -261,9 +258,9 @@ rust_sched_loop::create_task(rust_task *spawner, const char *name) {
     rust_task *task =
         new (this->kernel, "rust_task")
         rust_task(this, task_state_newborn,
-                  spawner, name, kernel->env->min_stack_size);
+                  name, kernel->env->min_stack_size);
     DLOG(this, task, "created task: " PTR ", spawner: %s, name: %s",
-                        task, spawner ? spawner->name : "null", name);
+                        task, spawner ? spawner->name : "(none)", name);
 
     task->id = kernel->generate_task_id();
     return task;

src/rt/rust_task.cpp

@@ -10,12 +10,9 @@
 #include "rust_env.h"
 #include "rust_port.h"
 
-// FIXME (#1789) (bblum): get rid of supervisors
-
 // Tasks
 rust_task::rust_task(rust_sched_loop *sched_loop, rust_task_state state,
-                     rust_task *spawner, const char *name,
-                     size_t init_stack_sz) :
+                     const char *name, size_t init_stack_sz) :
     ref_count(1),
     id(0),
     notify_enabled(false),
@@ -30,7 +27,6 @@ rust_task::rust_task(rust_sched_loop *sched_loop, rust_task_state state,
     local_region(&sched_loop->local_region),
     boxed(sched_loop->kernel->env, &local_region),
     unwinding(false),
-    propagate_failure(true),
     cc_counter(0),
     total_stack_sz(0),
     task_local_data(NULL),
@@ -45,17 +41,13 @@ rust_task::rust_task(rust_sched_loop *sched_loop, rust_task_state state,
     disallow_kill(0),
     c_stack(NULL),
     next_c_sp(0),
-    next_rust_sp(0),
-    supervisor(spawner)
+    next_rust_sp(0)
 {
     LOGPTR(sched_loop, "new task", (uintptr_t)this);
     DLOG(sched_loop, task, "sizeof(task) = %d (0x%x)",
          sizeof *this, sizeof *this);
 
     new_stack(init_stack_sz);
-    if (supervisor) {
-        supervisor->ref();
-    }
 }
 
 // NB: This does not always run on the task's scheduler thread
@@ -65,15 +57,6 @@ rust_task::delete_this()
     DLOG(sched_loop, task, "~rust_task %s @0x%" PRIxPTR ", refcnt=%d",
          name, (uintptr_t)this, ref_count);
 
-    // FIXME (#2677): We should do this when the task exits, not in the
-    // destructor
-    {
-        scoped_lock with(supervisor_lock);
-        if (supervisor) {
-            supervisor->deref();
-        }
-    }
-
     /* FIXME (#2677): tighten this up, there are some more
        assertions that hold at task-lifecycle events. */
     assert(ref_count == 0); // ||
@@ -335,21 +318,6 @@ void rust_task::fail_sched_loop() {
     sched_loop->fail();
 }
 
-void
-rust_task::unsupervise()
-{
-    scoped_lock with(supervisor_lock);
-    if (supervisor) {
-        DLOG(sched_loop, task,
-             "task %s @0x%" PRIxPTR
-             " disconnecting from supervisor %s @0x%" PRIxPTR,
-             name, this, supervisor->name, supervisor);
-        supervisor->deref();
-    }
-    supervisor = NULL;
-    propagate_failure = false;
-}
-
 frame_glue_fns*
 rust_task::get_frame_glue_fns(uintptr_t fp) {
     fp -= sizeof(uintptr_t);

src/rt/rust_task.h

@@ -122,6 +122,8 @@ rust_task_fail(rust_task *task,
 struct
 rust_task : public kernel_owned<rust_task>
 {
+    // FIXME(#1789) Refcounting no longer seems needed now that tasks don't
+    // ref their parents. I'll leave it in just in case... -- bblum
     RUST_ATOMIC_REFCOUNT();
 
     rust_task_id id;
@@ -193,9 +195,6 @@ rust_task : public kernel_owned<rust_task>
 
     rust_port_selector port_selector;
 
-    lock_and_signal supervisor_lock;
-    rust_task *supervisor;     // Parent-link for failure propagation.
-
     // Called when the atomic refcount reaches zero
     void delete_this();
 
@@ -237,7 +236,6 @@ rust_task : public kernel_owned<rust_task>
     // Only a pointer to 'name' is kept, so it must live as long as this task.
     rust_task(rust_sched_loop *sched_loop,
               rust_task_state state,
-              rust_task *spawner,
               const char *name,
               size_t init_stack_sz);
 
@@ -279,9 +277,6 @@ rust_task : public kernel_owned<rust_task>
     // FIXME (#1868) (bblum): maybe this can be done at rust-level?
     void fail_sched_loop();
 
-    // Disconnect from our supervisor.
-    void unsupervise();
-
     frame_glue_fns *get_frame_glue_fns(uintptr_t fp);
 
     void *calloc(size_t size, const char *tag);