From 4cf6b4d3b4ea5cd231ab96d82f5f8cd794e0b2c3 Mon Sep 17 00:00:00 2001 From: Ben Blum Date: Tue, 17 Jul 2012 20:40:40 -0400 Subject: [PATCH] Tasks should not hold a ref to their parent (Close #1789) --- src/libcore/sys.rs | 1 - src/libcore/task.rs | 26 +++++++++++++++++--------- src/rt/rust_builtin.cpp | 3 +-- src/rt/rust_sched_loop.cpp | 7 ++----- src/rt/rust_task.cpp | 36 ++---------------------------------- src/rt/rust_task.h | 9 ++------- 6 files changed, 24 insertions(+), 58 deletions(-) diff --git a/src/libcore/sys.rs b/src/libcore/sys.rs index 0e5a39e859e5e..7a23f0c3e0b4d 100644 --- a/src/libcore/sys.rs +++ b/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; diff --git a/src/libcore/task.rs b/src/libcore/task.rs index 0efc15cda2ad4..7f798dcf04798 100644 --- a/src/libcore/task.rs +++ b/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,8 +749,7 @@ 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) @@ -759,10 +757,6 @@ fn spawn_raw(opts: task_opts, +f: fn~()) { *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) { } diff --git a/src/rt/rust_builtin.cpp b/src/rt/rust_builtin.cpp index 4975bbf03544d..f86e9232625ce 100644 --- a/src/rt/rust_builtin.cpp +++ b/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 diff --git a/src/rt/rust_sched_loop.cpp b/src/rt/rust_sched_loop.cpp index 1a63596f15c0d..1fbc0c1e07c52 100644 --- a/src/rt/rust_sched_loop.cpp +++ b/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; diff --git a/src/rt/rust_task.cpp b/src/rt/rust_task.cpp index 64ee56a5115e5..84984d499e9a5 100644 --- a/src/rt/rust_task.cpp +++ b/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); diff --git a/src/rt/rust_task.h b/src/rt/rust_task.h index 8baa852378a84..cae058d7227d2 100644 --- a/src/rt/rust_task.h +++ b/src/rt/rust_task.h @@ -122,6 +122,8 @@ rust_task_fail(rust_task *task, struct rust_task : public kernel_owned { + // 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_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 // 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 // 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);