Rework the idle callback to have a safer interface

It turns out that the uv implementation would cause use-after-free if the idle
callback was used after the call to `close`, and additionally nothing would ever
really work that well if `start()` were called twice. To change this, the
`start` and `close` methods were removed in favor of specifying the callback at
creation, and allowing destruction to take care of closing the watcher.

Author: alexcrichton

src/librustuv/idle.rs

@@ -19,11 +19,11 @@ pub struct IdleWatcher {
     handle: *uvll::uv_idle_t,
     idle_flag: bool,
     closed: bool,
-    callback: Option<~Callback>,
+    callback: ~Callback,
 }
 
 impl IdleWatcher {
-    pub fn new(loop_: &mut Loop) -> ~IdleWatcher {
+    pub fn new(loop_: &mut Loop, cb: ~Callback) -> ~IdleWatcher {
         let handle = UvHandle::alloc(None::<IdleWatcher>, uvll::UV_IDLE);
         assert_eq!(unsafe {
             uvll::uv_idle_init(loop_.handle, handle)
@@ -32,7 +32,7 @@ impl IdleWatcher {
             handle: handle,
             idle_flag: false,
             closed: false,
-            callback: None,
+            callback: cb,
         };
         return me.install();
     }
@@ -64,12 +64,6 @@ impl IdleWatcher {
 }
 
 impl PausibleIdleCallback for IdleWatcher {
-    fn start(&mut self, cb: ~Callback) {
-        assert!(self.callback.is_none());
-        self.callback = Some(cb);
-        assert_eq!(unsafe { uvll::uv_idle_start(self.handle, idle_cb) }, 0)
-        self.idle_flag = true;
-    }
     fn pause(&mut self) {
         if self.idle_flag == true {
             assert_eq!(unsafe {uvll::uv_idle_stop(self.handle) }, 0);
@@ -82,84 +76,93 @@ impl PausibleIdleCallback for IdleWatcher {
             self.idle_flag = true;
         }
     }
-    fn close(&mut self) {
-        self.pause();
-        if !self.closed {
-            self.closed = true;
-            self.close_async_();
-        }
-    }
 }
 
 impl UvHandle<uvll::uv_idle_t> for IdleWatcher {
     fn uv_handle(&self) -> *uvll::uv_idle_t { self.handle }
 }
 
 extern fn idle_cb(handle: *uvll::uv_idle_t, status: c_int) {
+    if status == uvll::ECANCELED { return }
     assert_eq!(status, 0);
     let idle: &mut IdleWatcher = unsafe { UvHandle::from_uv_handle(&handle) };
-    assert!(idle.callback.is_some());
-    idle.callback.get_mut_ref().call();
+    idle.callback.call();
+}
+
+impl Drop for IdleWatcher {
+    fn drop(&mut self) {
+        self.pause();
+        self.close_async_();
+    }
 }
 
 #[cfg(test)]
 mod test {
-
-    use Loop;
     use super::*;
-    use std::unstable::run_in_bare_thread;
+    use std::rt::tube::Tube;
+    use std::rt::rtio::{Callback, PausibleIdleCallback};
+    use super::super::run_uv_loop;
+
+    struct MyCallback(Tube<int>, int);
+    impl Callback for MyCallback {
+        fn call(&mut self) {
+            match *self {
+                MyCallback(ref mut tube, val) => tube.send(val)
+            }
+        }
+    }
 
     #[test]
-    #[ignore(reason = "valgrind - loop destroyed before watcher?")]
-    fn idle_new_then_close() {
-        do run_in_bare_thread {
-            let mut loop_ = Loop::new();
-            let idle_watcher = { IdleWatcher::new(&mut loop_) };
-            idle_watcher.close(||());
+    fn not_used() {
+        do run_uv_loop |l| {
+            let cb = ~MyCallback(Tube::new(), 1);
+            let _idle = IdleWatcher::new(l, cb as ~Callback);
         }
     }
 
     #[test]
-    fn idle_smoke_test() {
-        do run_in_bare_thread {
-            let mut loop_ = Loop::new();
-            let mut idle_watcher = { IdleWatcher::new(&mut loop_) };
-            let mut count = 10;
-            let count_ptr: *mut int = &mut count;
-            do idle_watcher.start |idle_watcher, status| {
-                let mut idle_watcher = idle_watcher;
-                assert!(status.is_none());
-                if unsafe { *count_ptr == 10 } {
-                    idle_watcher.stop();
-                    idle_watcher.close(||());
-                } else {
-                    unsafe { *count_ptr = *count_ptr + 1; }
-                }
-            }
-            loop_.run();
-            loop_.close();
-            assert_eq!(count, 10);
+    fn smoke_test() {
+        do run_uv_loop |l| {
+            let mut tube = Tube::new();
+            let cb = ~MyCallback(tube.clone(), 1);
+            let mut idle = IdleWatcher::new(l, cb as ~Callback);
+            idle.resume();
+            tube.recv();
         }
     }
 
     #[test]
-    fn idle_start_stop_start() {
-        do run_in_bare_thread {
-            let mut loop_ = Loop::new();
-            let mut idle_watcher = { IdleWatcher::new(&mut loop_) };
-            do idle_watcher.start |idle_watcher, status| {
-                let mut idle_watcher = idle_watcher;
-                assert!(status.is_none());
-                idle_watcher.stop();
-                do idle_watcher.start |idle_watcher, status| {
-                    assert!(status.is_none());
-                    let mut idle_watcher = idle_watcher;
-                    idle_watcher.stop();
-                    idle_watcher.close(||());
-                }
-            }
-            loop_.run();
-            loop_.close();
+    fn fun_combinations_of_methods() {
+        do run_uv_loop |l| {
+            let mut tube = Tube::new();
+            let cb = ~MyCallback(tube.clone(), 1);
+            let mut idle = IdleWatcher::new(l, cb as ~Callback);
+            idle.resume();
+            tube.recv();
+            idle.pause();
+            idle.resume();
+            idle.resume();
+            tube.recv();
+            idle.pause();
+            idle.pause();
+            idle.resume();
+            tube.recv();
+        }
+    }
+
+    #[test]
+    fn pause_pauses() {
+        do run_uv_loop |l| {
+            let mut tube = Tube::new();
+            let cb = ~MyCallback(tube.clone(), 1);
+            let mut idle1 = IdleWatcher::new(l, cb as ~Callback);
+            let cb = ~MyCallback(tube.clone(), 2);
+            let mut idle2 = IdleWatcher::new(l, cb as ~Callback);
+            idle2.resume();
+            assert_eq!(tube.recv(), 2);
+            idle2.pause();
+            idle1.resume();
+            assert_eq!(tube.recv(), 1);
         }
     }
 }

src/librustuv/uvio.rs

@@ -148,8 +148,8 @@ impl EventLoop for UvEventLoop {
         IdleWatcher::onetime(self.uvio.uv_loop(), f);
     }
 
-    fn pausible_idle_callback(&mut self) -> ~PausibleIdleCallback {
-        IdleWatcher::new(self.uvio.uv_loop()) as ~PausibleIdleCallback
+    fn pausible_idle_callback(&mut self, cb: ~Callback) -> ~PausibleIdleCallback {
+        IdleWatcher::new(self.uvio.uv_loop(), cb) as ~PausibleIdleCallback
     }
 
     fn remote_callback(&mut self, f: ~Callback) -> ~RemoteCallback {

src/libstd/rt/basic.rs

@@ -107,7 +107,7 @@ impl BasicLoop {
             match self.idle {
                 Some(idle) => {
                     if (*idle).active {
-                        (*idle).work.get_mut_ref().call();
+                        (*idle).work.call();
                     }
                 }
                 None => {}
@@ -150,8 +150,8 @@ impl EventLoop for BasicLoop {
     }
 
     // XXX: Seems like a really weird requirement to have an event loop provide.
-    fn pausible_idle_callback(&mut self) -> ~PausibleIdleCallback {
-        let callback = ~BasicPausible::new(self);
+    fn pausible_idle_callback(&mut self, cb: ~Callback) -> ~PausibleIdleCallback {
+        let callback = ~BasicPausible::new(self, cb);
         rtassert!(self.idle.is_none());
         unsafe {
             let cb_ptr: &*mut BasicPausible = cast::transmute(&callback);
@@ -204,36 +204,27 @@ impl Drop for BasicRemote {
 
 struct BasicPausible {
     eloop: *mut BasicLoop,
-    work: Option<~Callback>,
+    work: ~Callback,
     active: bool,
 }
 
 impl BasicPausible {
-    fn new(eloop: &mut BasicLoop) -> BasicPausible {
+    fn new(eloop: &mut BasicLoop, cb: ~Callback) -> BasicPausible {
         BasicPausible {
             active: false,
-            work: None,
+            work: cb,
             eloop: eloop,
         }
     }
 }
 
 impl PausibleIdleCallback for BasicPausible {
-    fn start(&mut self, f: ~Callback) {
-        rtassert!(!self.active && self.work.is_none());
-        self.active = true;
-        self.work = Some(f);
-    }
     fn pause(&mut self) {
         self.active = false;
     }
     fn resume(&mut self) {
         self.active = true;
     }
-    fn close(&mut self) {
-        self.active = false;
-        self.work = None;
-    }
 }
 
 impl Drop for BasicPausible {

src/libstd/rt/rtio.rs

@@ -31,7 +31,7 @@ pub trait Callback {
 pub trait EventLoop {
     fn run(&mut self);
     fn callback(&mut self, proc());
-    fn pausible_idle_callback(&mut self) -> ~PausibleIdleCallback;
+    fn pausible_idle_callback(&mut self, ~Callback) -> ~PausibleIdleCallback;
     fn remote_callback(&mut self, ~Callback) -> ~RemoteCallback;
 
     /// The asynchronous I/O services. Not all event loops may provide one
@@ -226,10 +226,8 @@ pub trait RtioTTY {
 }
 
 pub trait PausibleIdleCallback {
-    fn start(&mut self, f: ~Callback);
     fn pause(&mut self);
     fn resume(&mut self);
-    fn close(&mut self);
 }
 
 pub trait RtioSignal {}

src/libstd/rt/sched.rs

@@ -169,7 +169,8 @@ impl Scheduler {
     pub fn bootstrap(mut ~self, task: ~Task) {
 
         // Build an Idle callback.
-        self.idle_callback = Some(self.event_loop.pausible_idle_callback());
+        let cb = ~SchedRunner as ~Callback;
+        self.idle_callback = Some(self.event_loop.pausible_idle_callback(cb));
 
         // Initialize the TLS key.
         local_ptr::init_tls_key();
@@ -184,7 +185,7 @@ impl Scheduler {
         // Before starting our first task, make sure the idle callback
         // is active. As we do not start in the sleep state this is
         // important.
-        self.idle_callback.get_mut_ref().start(~SchedRunner as ~Callback);
+        self.idle_callback.get_mut_ref().resume();
 
         // Now, as far as all the scheduler state is concerned, we are
         // inside the "scheduler" context. So we can act like the
@@ -202,7 +203,7 @@ impl Scheduler {
 
         // Close the idle callback.
         let mut sched: ~Scheduler = Local::take();
-        sched.idle_callback.get_mut_ref().close();
+        sched.idle_callback.take();
         // Make one go through the loop to run the close callback.
         sched.run();