Feedback about cancelable sleep module

[hasunwoo]

I wrote module that provides cancelable sleep using Condvar. I want to know if there is any room for improvements(coding style, idiomatic rust, deadlock possibility, ...).

use std::{
    sync::{Arc, Condvar, Mutex},
    time::{Duration, Instant},
};

struct Inner {
    is_canceled: Arc<(Mutex<bool>, Condvar)>,
    wakeup_time: Instant,
}

pub fn sleep(duration: Duration) -> (SleepHandle, CancelHandle) {
    let inner = Inner {
        is_canceled: Arc::new((Mutex::new(false), Condvar::new())),
        wakeup_time: Instant::now() + duration,
    };
    (
        SleepHandle::from_inner(&inner),
        CancelHandle::from_inner(&inner),
    )
}

pub struct SleepHandle(Inner);

impl SleepHandle {
    pub fn sleep(self) {
        let (lock, cvar) = &*self.0.is_canceled;
        let mut is_canceled = lock.lock().unwrap();
        while !*is_canceled {
            let duration_left = self.0.wakeup_time.checked_duration_since(Instant::now());
            match duration_left {
                Some(duration_left) => {
                    let result = cvar
                        .wait_timeout_while(is_canceled, duration_left, |&mut is_canceled| {
                            !is_canceled
                        })
                        .unwrap();
                    is_canceled = result.0;
                }
                None => break,
            }
        }
    }

    fn from_inner(inner: &Inner) -> Self {
        Self(Inner {
            is_canceled: Arc::clone(&inner.is_canceled),
            wakeup_time: inner.wakeup_time,
        })
    }
}

pub struct CancelHandle(Inner);

impl CancelHandle {
    pub fn cancel(self) {
        let (lock, cvar) = &*self.0.is_canceled;
        let mut is_canceled = lock.lock().unwrap();
        *is_canceled = true;
        cvar.notify_all();
    }

    fn from_inner(inner: &Inner) -> Self {
        Self(Inner {
            is_canceled: Arc::clone(&inner.is_canceled),
            wakeup_time: inner.wakeup_time,
        })
    }
}

impl Clone for CancelHandle {
    fn clone(&self) -> Self {
        CancelHandle::from_inner(&self.0)
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::{thread, time::Instant};

    #[test]
    fn test_sleep() {
        let duration = Duration::from_millis(1300);
        let (sleep, _) = sleep(duration);
        let before = Instant::now();
        thread::spawn(move || {
            sleep.sleep();
        })
        .join()
        .unwrap();
        assert!(before.elapsed().as_millis().abs_diff(duration.as_millis()) < 50);
    }

    #[test]
    fn test_cancel() {
        let duration = Duration::from_millis(8000);
        let cancel_after = Duration::from_millis(3421);
        let (sleep, cancel) = sleep(duration);
        let before = Instant::now();
        thread::spawn(move || {
            let before = Instant::now();
            std::thread::sleep(cancel_after);
            cancel.cancel();
            //assert that cancel does not block
            assert!(
                before
                    .elapsed()
                    .as_millis()
                    .abs_diff(cancel_after.as_millis())
                    < 50
            );
        });
        thread::spawn(move || {
            sleep.sleep();
        })
        .join()
        .unwrap();
        assert!(
            before
                .elapsed()
                .as_millis()
                .abs_diff(cancel_after.as_millis())
                < 50
        );
    }

    #[test]
    fn test_multiple_cancel() {
        let duration = Duration::from_millis(8000);
        let cancel_after = Duration::from_millis(3421);
        let (sleep, cancel) = sleep(duration);
        let before = Instant::now();
        {
            let cancel = cancel.clone();
            thread::spawn(move || {
                let before = Instant::now();
                std::thread::sleep(cancel_after);
                cancel.cancel();
                //assert that cancel does not block
                assert!(
                    before
                        .elapsed()
                        .as_millis()
                        .abs_diff(cancel_after.as_millis())
                        < 50
                );
            });
        }
        thread::spawn(move || {
            let cancel_after = Duration::from_millis(5400);
            let before = Instant::now();
            std::thread::sleep(cancel_after);
            cancel.cancel();
            //assert that second cancel does not block
            assert!(
                before
                    .elapsed()
                    .as_millis()
                    .abs_diff(cancel_after.as_millis())
                    < 50
            );
        });
        thread::spawn(move || {
            sleep.sleep();
        })
        .join()
        .unwrap();
        assert!(
            before
                .elapsed()
                .as_millis()
                .abs_diff(cancel_after.as_millis())
                < 50
        );
    }

    #[test]
    fn test_sleep_after_cancel() {
        let before = Instant::now();
        let (sleep, cancel) = sleep(Duration::from_millis(5000));
        cancel.cancel();
        sleep.sleep();
        assert!(before.elapsed().as_millis() < 50);
    }
}

[Yandros]

The logic looks good to me :

• For instance, I don't see any danger for a deadlock since the lock guards are always held in a short-lived fashion / with no arbitrary or complex code run while holding it;

Idiomacy-wise, my main tip here would be to forgo the from_inner constructors, since they are just private, and equivalent to a #[derive(Clone)] on Inner.

• Bonus: we can then replace the manual Clone impl with another #[derive(Clone)].

• Super tiny extra bonus: we get to save on a Arc::clone() of inner thanks to this loss of abstraction.

The reason for doing this is to keep things simple (KISS, and without hindering DRY in this case), since the code is short enough to justify not splitting across files and abstractions.

@@ -3,6 +3,7 @@ use std::{
     time::{Duration, Instant},
 };
 
+#[derive(Clone)]
 struct Inner {
     is_canceled: Arc<(Mutex<bool>, Condvar)>,
     wakeup_time: Instant,
@@ -14,8 +15,8 @@ pub fn sleep(duration: Duration) -> (SleepHandle, CancelHandle) {
         wakeup_time: Instant::now() + duration,
     };
     (
-        SleepHandle::from_inner(&inner),
-        CancelHandle::from_inner(&inner),
+        SleepHandle(inner.clone()),
+        CancelHandle(inner),
     )
 }
 
@@ -40,15 +41,9 @@ impl SleepHandle {
             }
         }
     }
-
-    fn from_inner(inner: &Inner) -> Self {
-        Self(Inner {
-            is_canceled: Arc::clone(&inner.is_canceled),
-            wakeup_time: inner.wakeup_time,
-        })
-    }
 }
 
+#[derive(Clone)]
 pub struct CancelHandle(Inner);
 
 impl CancelHandle {
@@ -58,19 +53,6 @@ impl CancelHandle {
         *is_canceled = true;
         cvar.notify_all();
     }
-
-    fn from_inner(inner: &Inner) -> Self {
-        Self(Inner {
-            is_canceled: Arc::clone(&inner.is_canceled),
-            wakeup_time: inner.wakeup_time,
-        })
-    }
-}
-
-impl Clone for CancelHandle {
-    fn clone(&self) -> Self {
-        CancelHandle::from_inner(&self.0)
-    }
 }
 
 #[cfg(test)]

• Playground

Another extra nit could be that the .cancel() method on the CancelHandle could just take a &self:

• making that change does not incur in extra runtime cost or operations within the function's body;

• CancelHandle : Clone, so the "affine type system" / move semantics of guaranteeing at most one cancel do not apply here (and given the logic in question, need not apply: it is fine for multiple actors to request a cancel).

---

A simpler implementation using channels

Note that your good snippet is doing things with some rather bare / low-level synchronization tools, such as Condvars (together with a Mutex and an Arc). It is a great educative experience, and potentially the starting point to trying to write or help maintain libraries specialized in these tasks.

But for one-off situations, I feel like it must be reminded that these "cancellable sleep" semantics are part of the API of the sdlib mpsc channel (even if probably under a slightly less efficient implementation due to their overly (to the task at hand) versatile API):

1. <mpsc::Receiver<()>>::recv_timeout() would be the core of your .sleep() function, with the Err(Timeout) case matching the uninterrupted sleep.

2. <mpsc::Sender<()>>::send(()) would be the core of your .cancel() function.

   • (or SyncSender if you want/need the CancelHandle to be Sync).

[hasunwoo]

thanks a lot