Unkillable is not unsafe. Close rust-lang#7832.

Author: bblum

src/libextra/sync.rs

@@ -130,11 +130,9 @@ impl<Q:Send> Sem<Q> {
 impl Sem<()> {
     pub fn access<U>(&self, blk: &fn() -> U) -> U {
         let mut release = None;
-        unsafe {
-            do task::unkillable {
-                self.acquire();
-                release = Some(SemRelease(self));
-            }
+        do task::unkillable {
+            self.acquire();
+            release = Some(SemRelease(self));
         }
         blk()
     }
@@ -153,11 +151,9 @@ impl Sem<~[WaitQueue]> {
 
     pub fn access_waitqueue<U>(&self, blk: &fn() -> U) -> U {
         let mut release = None;
-        unsafe {
-            do task::unkillable {
-                self.acquire();
-                release = Some(SemAndSignalRelease(self));
-            }
+        do task::unkillable {
+            self.acquire();
+            release = Some(SemAndSignalRelease(self));
         }
         blk()
     }
@@ -294,17 +290,15 @@ impl<'self> Condvar<'self> {
         #[unsafe_destructor]
         impl<'self> Drop for CondvarReacquire<'self> {
             fn drop(&self) {
-                unsafe {
-                    // Needs to succeed, instead of itself dying.
-                    do task::unkillable {
-                        match self.order {
-                            Just(lock) => do lock.access {
-                                self.sem.acquire();
-                            },
-                            Nothing => {
-                                self.sem.acquire();
-                            },
-                        }
+                // Needs to succeed, instead of itself dying.
+                do task::unkillable {
+                    match self.order {
+                        Just(lock) => do lock.access {
+                            self.sem.acquire();
+                        },
+                        Nothing => {
+                            self.sem.acquire();
+                        },
                     }
                 }
             }
@@ -644,14 +638,12 @@ impl RWLock {
         // Implementation slightly different from the slicker 'write's above.
         // The exit path is conditional on whether the caller downgrades.
         let mut _release = None;
-        unsafe {
-            do task::unkillable {
-                (&self.order_lock).acquire();
-                (&self.access_lock).acquire();
-                (&self.order_lock).release();
-            }
-            _release = Some(RWLockReleaseDowngrade(self));
+        do task::unkillable {
+            (&self.order_lock).acquire();
+            (&self.access_lock).acquire();
+            (&self.order_lock).release();
         }
+        _release = Some(RWLockReleaseDowngrade(self));
         blk(RWLockWriteMode { lock: self })
     }
 

src/libstd/rt/select.rs

@@ -208,7 +208,7 @@ mod test {
     #[test]
     fn select_unkillable() {
         do run_in_newsched_task {
-            unsafe { do task::unkillable { select_helper(2, [1]) } }
+            do task::unkillable { select_helper(2, [1]) }
         }
     }
 
@@ -243,7 +243,7 @@ mod test {
                 if killable {
                     assert!(select(ports) == 1);
                 } else {
-                    unsafe { do task::unkillable { assert!(select(ports) == 1); } }
+                    do task::unkillable { assert!(select(ports) == 1); }
                 }
             }
         }
@@ -287,7 +287,7 @@ mod test {
                         if killable {
                             select(ports);
                         } else {
-                            unsafe { do task::unkillable { select(ports); } }
+                            do task::unkillable { select(ports); }
                         }
                     }
                 }
@@ -301,27 +301,25 @@ mod test {
             let (success_p, success_c) = oneshot::<bool>();
             let success_c = Cell::new(success_c);
             do task::try {
-                unsafe {
-                    let success_c = Cell::new(success_c.take());
-                    do task::unkillable {
-                        let (p,c) = oneshot();
-                        let c = Cell::new(c);
-                        do task::spawn {
-                            let (dead_ps, dead_cs) = unzip(from_fn(5, |_| oneshot::<()>()));
-                            let mut ports = dead_ps;
-                            select(ports); // should get killed; nothing should leak
-                            c.take().send(()); // must not happen
-                            // Make sure dead_cs doesn't get closed until after select.
-                            let _ = dead_cs;
-                        }
-                        do task::spawn {
-                            fail!(); // should kill sibling awake
-                        }
-
-                        // wait for killed selector to close (NOT send on) its c.
-                        // hope to send 'true'.
-                        success_c.take().send(p.try_recv().is_none());
+                let success_c = Cell::new(success_c.take());
+                do task::unkillable {
+                    let (p,c) = oneshot();
+                    let c = Cell::new(c);
+                    do task::spawn {
+                        let (dead_ps, dead_cs) = unzip(from_fn(5, |_| oneshot::<()>()));
+                        let mut ports = dead_ps;
+                        select(ports); // should get killed; nothing should leak
+                        c.take().send(()); // must not happen
+                        // Make sure dead_cs doesn't get closed until after select.
+                        let _ = dead_cs;
                     }
+                    do task::spawn {
+                        fail!(); // should kill sibling awake
+                    }
+
+                    // wait for killed selector to close (NOT send on) its c.
+                    // hope to send 'true'.
+                    success_c.take().send(p.try_recv().is_none());
                 }
             };
             assert!(success_p.recv());

src/libstd/task/mod.rs

@@ -618,32 +618,34 @@ pub fn get_scheduler() -> Scheduler {
  * }
  * ~~~
  */
-pub unsafe fn unkillable<U>(f: &fn() -> U) -> U {
+pub fn unkillable<U>(f: &fn() -> U) -> U {
     use rt::task::Task;
 
-    match context() {
-        OldTaskContext => {
-            let t = rt::rust_get_task();
-            do (|| {
-                rt::rust_task_inhibit_kill(t);
-                f()
-            }).finally {
-                rt::rust_task_allow_kill(t);
+    unsafe {
+        match context() {
+            OldTaskContext => {
+                let t = rt::rust_get_task();
+                do (|| {
+                    rt::rust_task_inhibit_kill(t);
+                    f()
+                }).finally {
+                    rt::rust_task_allow_kill(t);
+                }
             }
-        }
-        TaskContext => {
-            // The inhibits/allows might fail and need to borrow the task.
-            let t = Local::unsafe_borrow::<Task>();
-            do (|| {
-                (*t).death.inhibit_kill((*t).unwinder.unwinding);
-                f()
-            }).finally {
-                (*t).death.allow_kill((*t).unwinder.unwinding);
+            TaskContext => {
+                // The inhibits/allows might fail and need to borrow the task.
+                let t = Local::unsafe_borrow::<Task>();
+                do (|| {
+                    (*t).death.inhibit_kill((*t).unwinder.unwinding);
+                    f()
+                }).finally {
+                    (*t).death.allow_kill((*t).unwinder.unwinding);
+                }
             }
+            // FIXME(#3095): This should be an rtabort as soon as the scheduler
+            // no longer uses a workqueue implemented with an Exclusive.
+            _ => f()
         }
-        // FIXME(#3095): This should be an rtabort as soon as the scheduler
-        // no longer uses a workqueue implemented with an Exclusive.
-        _ => f()
     }
 }
 

src/libstd/task/spawn.rs

@@ -694,7 +694,7 @@ fn spawn_raw_newsched(mut opts: TaskOpts, f: ~fn()) {
         // Should be run after the local-borrowed task is returned.
         if enlist_success {
             if indestructible {
-                unsafe { do unkillable { f() } }
+                do unkillable { f() }
             } else {
                 f()
             }