diff --git a/flake.nix b/flake.nix index 8bfecb8..c7aeea2 100644 --- a/flake.nix +++ b/flake.nix @@ -34,7 +34,7 @@ # `nix develop` devShell = pkgs.mkShell { buildInputs = buildInputs; - nativeBuildInputs = nativeBuildInputs ++ [ pkgs.rustc pkgs.cargo ]; + nativeBuildInputs = nativeBuildInputs ++ (with pkgs; [ rustc cargo gnome3.zenity ]); }; }); } diff --git a/test-client.sh b/test-client.sh index 579c2c4..c46fc56 100755 --- a/test-client.sh +++ b/test-client.sh @@ -1,7 +1,6 @@ -#!/bin/sh -# -*- eval: (aggressive-indent-mode -1); -*- +#!/usr/bin/env bash -set -e +set -euo pipefail alias woot='cargo run --bin xidlehook-client -- --socket "/tmp/xidlehook-test.sock"' diff --git a/test-daemon.sh b/test-daemon.sh index d0fa4de..28c0b95 100755 --- a/test-daemon.sh +++ b/test-daemon.sh @@ -1,9 +1,9 @@ -#!/bin/sh -# -*- eval: (aggressive-indent-mode -1); -*- +#!/usr/bin/env bash -set -e +set -euo pipefail cargo run -- \ + --timer 1 "echo start sleep; sleep 5; echo end sleep" "" \ --timer 2 "echo 2" "" \ --timer 1 "echo 3" "" \ --timer 1 "echo 4" "" \ diff --git a/test-usecase.sh b/test-usecase.sh new file mode 100755 index 0000000..afe425f --- /dev/null +++ b/test-usecase.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash + +set -euo pipefail + +pipe="$(mktemp -d)/pipe" + +mkfifo "$pipe" + +read -rd '' lock < $pipe" "" \ + --timer 2 "echo 20 > $pipe" "" \ + --timer 2 "echo 40 > $pipe" "" \ + --timer 2 "echo 60 > $pipe" "" \ + --timer 2 "echo 80 > $pipe" "" \ + --timer 2 "echo 100 > $pipe" "" \ + --timer 2 "$suspend" "" diff --git a/xidlehook-core/src/lib.rs b/xidlehook-core/src/lib.rs index 69eba26..dfa5977 100644 --- a/xidlehook-core/src/lib.rs +++ b/xidlehook-core/src/lib.rs @@ -19,6 +19,10 @@ #![allow( // I don't agree with this lint clippy::must_use_candidate, + + // The integer arithmetic here is mostly regarding indexes into Vecs, indexes where memory + // allocation will fail far, far earlier than the arithmetic will fail. + clippy::integer_arithmetic, )] //! Instead of implementing your extension as something that @@ -204,6 +208,8 @@ where pub fn reset(&mut self) -> Result<()> { self.abort()?; + trace!("Resetting"); + if self.next_index > 0 { if let Err(err) = self.module.reset() { self.module.warning(&err)?; @@ -218,18 +224,6 @@ where Ok(()) } - fn next_enabled(&'_ mut self, mut index: usize) -> Option { - while self.timers.get_mut(index)?.disabled() { - trace!("Timer {} was disabled, going to next...", index); - // Thanks, clippy, but get_mut will fail far before this is even close to overflowing - #[allow(clippy::integer_arithmetic)] - { - index += 1; - } - } - Some(index) - } - /// Skip ahead to the selected timer. Timers leading up to this point will not be ran. If you /// pass `force`, modules will not even be able to prevent this from happening (all requests /// pre-timer would be ignored). Post-timer requests are fully complied with. @@ -304,15 +298,7 @@ where } // Next time, continue from next index - // - // Thanks, clippy, but get_mut will fail far before this is even close to - // overflowing - #[allow(clippy::integer_arithmetic)] - { - self.next_index = self - .next_enabled(index + 1) - .unwrap_or_else(|| self.timers.len()); - } + self.next_index = index + 1; Ok(Progress::Continue) } @@ -329,25 +315,42 @@ where self.previous_idle_time = absolute_time; - let first_index = match self.next_enabled(0) { - Some(index) => index, - - // There are no enabled timers... As requested by user @desbma on GitHub, it makes most - // sense to leave xidlehook doing nothing, as it can still be activated using other - // means such as the socket API. See the message: - // https://github.com/jD91mZM2/xidlehook/issues/35#issuecomment-579495447 - None => return Ok(Action::Forever), - }; - // We can only ever sleep as long as it takes for the first timer to activate, since the // user may become active (and then idle again) at any point. - let mut max_sleep = self.timers[first_index] - .time_left(Duration::default())? - .unwrap_or_default(); - trace!( - "Taking the first timer into account. Remaining: {:?}", - max_sleep - ); + let mut max_sleep = Duration::from_nanos(u64::MAX); + + let mut first_timer = 0; + + while let Some(timer) = self.timers.get_mut(first_timer) { + if !timer.disabled() { + break; + } + + // This timer may re-activate in the future and take presedence over the timer we + // thought was the next enabled timer. + if let Some(remaining) = timer.time_left(Duration::from_nanos(0))? { + trace!( + "Taking disabled first timer into account. Remaining: {:?}", + remaining + ); + max_sleep = cmp::min(max_sleep, remaining); + } + + first_timer += 1; + } + + if let Some(timer) = self.timers.get_mut(first_timer) { + if let Some(remaining) = timer.time_left(Duration::from_nanos(0))? { + trace!( + "Taking first timer into account. Remaining: {:?}", + remaining + ); + max_sleep = cmp::min(max_sleep, remaining) + } + } else { + // No timer was enabled! + return Ok(Action::Forever); + } if self.aborted { trace!("This chain was aborted, I won't pursue it"); @@ -357,44 +360,44 @@ where let relative_time = absolute_time - self.base_idle_time; trace!("Relative time: {:?}", relative_time); - if self.next_index == 0 { - // Normally the check for whether a timer is disabled or not is done when calculating - // the time until the next timer should pop. But in the case of the first timer being - // disabled, well, there's no previous timer to have done this check. So let's skip - // ahead. - // - // Note: We don't need to worry about what could happen if a timer is disabled midway, - // see the docs for `disabled` - it's expected to ignore these changes. - self.next_index = first_index; + let mut next_index = self.next_index; + + while let Some(timer) = self.timers.get_mut(next_index) { + if !timer.disabled() { + break; + } + + // This timer may re-activate in the future and take presedence over the timer we + // thought was the next enabled timer. + if let Some(remaining) = timer.time_left(relative_time)? { + trace!( + "Taking disabled timer into account. Remaining: {:?}", + remaining + ); + max_sleep = cmp::min(max_sleep, remaining); + } + + next_index += 1; } // When there's a next timer available, get the time until that activates - if let Some(next) = self.timers.get_mut(self.next_index) { + if let Some(next) = self.timers.get_mut(next_index) { if let Some(remaining) = next.time_left(relative_time)? { - trace!("Taking next timer into account. Remaining: {:?}", remaining); + trace!( + "Taking next enabled timer into account. Remaining: {:?}", + remaining + ); max_sleep = cmp::min(max_sleep, remaining); } else { + trace!("Triggering timer #{}", next_index); // Oh! It has already been passed - let's trigger it. - match self.trigger(self.next_index, absolute_time, false)? { - Progress::Continue => (), - Progress::Abort => return Ok(Action::Sleep(max_sleep)), - Progress::Reset => return Ok(Action::Sleep(max_sleep)), + match self.trigger(next_index, absolute_time, false)? { Progress::Stop => return Ok(Action::Quit), + _ => (), } - // From now on, `relative_time` is outdated. Don't use it. - - if let Some(next) = self.timers.get_mut(self.next_index) { - assert!(!next.disabled()); - if let Some(remaining) = next.time_left(Duration::default())? { - // We can't sleep longer than it will take for the next timer to trigger - max_sleep = cmp::min(max_sleep, remaining); - trace!( - "Taking next-next timer into account. Remaining: {:?}", - remaining - ); - } - } + // Recurse to find return value + return self.poll(absolute_time); } } diff --git a/xidlehook-core/src/timers.rs b/xidlehook-core/src/timers.rs index 2ed6960..cf0933d 100644 --- a/xidlehook-core/src/timers.rs +++ b/xidlehook-core/src/timers.rs @@ -1,7 +1,10 @@ //! The timer trait and some useful implementations use crate::Result; -use std::{process::Command, time::Duration}; +use std::{ + process::{Child, Command}, + time::Duration, +}; /// The timer trait is used to tell xidlehook after how much idle time /// your timer should activate (relatively), and what activation @@ -11,10 +14,9 @@ use std::{process::Command, time::Duration}; pub trait Timer { /// Return the time left based on the relative idle time fn time_left(&mut self, idle_time: Duration) -> Result>; - /// How urgent this timer wants to be notified on abort (when the - /// user is no longer idle). Return as slow of a duration as you - /// think is acceptable to be nice to the CPU - preferrably return - /// `None` which basically means infinity. + /// How urgent this timer wants to be notified on abort (when the user is no longer idle). + /// Return as slow of a duration as you think is acceptable to be nice to the CPU - preferrably + /// return `None` which basically means infinity. fn abort_urgency(&self) -> Option { None } @@ -32,10 +34,9 @@ pub trait Timer { fn deactivate(&mut self) -> Result<()> { Ok(()) } - /// Return true if the timer is disabled and should be - /// skipped. This function is called immediately after the - /// previous timer is triggered, so any changes since then aren't - /// reflected. + /// Return true if the timer is disabled and should be skipped. Changes to this value are + /// reflected - you may enable a timer that was previously disabled, and xidlehook will call it + /// as soon as the timer is passed - or immediately if the timer has already passed. fn disabled(&mut self) -> bool { false } @@ -55,6 +56,9 @@ pub struct CmdTimer { pub deactivation: Option, /// Whether or not to disable this timer pub disabled: bool, + + /// The child process that is currently running + pub activation_child: Option, } impl Timer for CmdTimer { fn time_left(&mut self, idle_time: Duration) -> Result> { @@ -70,7 +74,7 @@ impl Timer for CmdTimer { fn activate(&mut self) -> Result<()> { if let Some(ref mut activation) = self.activation { - activation.spawn()?; + self.activation_child = Some(activation.spawn()?); } Ok(()) } @@ -87,7 +91,13 @@ impl Timer for CmdTimer { Ok(()) } fn disabled(&mut self) -> bool { - self.disabled + if let Some(Ok(None)) = self.activation_child.as_mut().map(|child| child.try_wait()) { + // We temporarily disable this timer while the child is still running + true + } else { + // Whether or not this command is disabled + self.disabled + } } } diff --git a/xidlehook-core/tests/timer.rs b/xidlehook-core/tests/disabled_timers.rs similarity index 52% rename from xidlehook-core/tests/timer.rs rename to xidlehook-core/tests/disabled_timers.rs index 34d2361..f9248ee 100644 --- a/xidlehook-core/tests/timer.rs +++ b/xidlehook-core/tests/disabled_timers.rs @@ -4,69 +4,7 @@ use xidlehook_core::{timers::CallbackTimer, Action::*, Xidlehook}; const TEST_UNIT: Duration = Duration::from_millis(50); #[test] -fn general_timer_test() { - let triggered = Cell::new(0); - - let mut timer = Xidlehook::new(vec![ - CallbackTimer::new(TEST_UNIT * 100, || triggered.set(triggered.get() | 1)), - CallbackTimer::new(TEST_UNIT * 010, || triggered.set(triggered.get() | 1 << 1)), - CallbackTimer::new(TEST_UNIT * 050, || triggered.set(triggered.get() | 1 << 2)), - CallbackTimer::new(TEST_UNIT * 200, || triggered.set(triggered.get() | 1 << 3)), - ]); - - // Test first timer - assert_eq!(timer.poll(TEST_UNIT * 0).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(timer.poll(TEST_UNIT * 20).unwrap(), Sleep(TEST_UNIT * 80)); - assert_eq!(timer.poll(TEST_UNIT * 40).unwrap(), Sleep(TEST_UNIT * 60)); - assert_eq!(timer.poll(TEST_UNIT * 74).unwrap(), Sleep(TEST_UNIT * 26)); - assert_eq!(timer.poll(TEST_UNIT * 99).unwrap(), Sleep(TEST_UNIT * 1)); - - // Trigger first timer - assert_eq!(triggered.get(), 0b0000); - assert_eq!(timer.poll(TEST_UNIT * 100).unwrap(), Sleep(TEST_UNIT * 10)); - assert_eq!(triggered.get(), 0b0001); - - // Test second timer - assert_eq!(timer.poll(TEST_UNIT * 103).unwrap(), Sleep(TEST_UNIT * 7)); - - // Overshoot second timer - assert_eq!(triggered.get(), 0b0001); - assert_eq!(timer.poll(TEST_UNIT * 500).unwrap(), Sleep(TEST_UNIT * 50)); - assert_eq!(triggered.get(), 0b0011); - - // Test third timer - assert_eq!(timer.poll(TEST_UNIT * 500).unwrap(), Sleep(TEST_UNIT * 50)); - assert_eq!(timer.poll(TEST_UNIT * 501).unwrap(), Sleep(TEST_UNIT * 49)); - assert_eq!(timer.poll(TEST_UNIT * 549).unwrap(), Sleep(TEST_UNIT * 1)); - - // Trigger third timer - assert_eq!(triggered.get(), 0b0011); - assert_eq!(timer.poll(TEST_UNIT * 550).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(triggered.get(), 0b0111); - - // Test fourth timer - triggered.set(0); - assert_eq!(timer.poll(TEST_UNIT * 600).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(timer.poll(TEST_UNIT * 649).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(timer.poll(TEST_UNIT * 650).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(triggered.get(), 0b0000); // no change - assert_eq!(timer.poll(TEST_UNIT * 680).unwrap(), Sleep(TEST_UNIT * 70)); - - // Trigger fourth timer - assert_eq!(triggered.get(), 0b0000); // no change - assert_eq!(timer.poll(TEST_UNIT * 750).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(triggered.get(), 0b1000); - - // It resets - triggered.set(0); - assert_eq!(timer.poll(TEST_UNIT * 0).unwrap(), Sleep(TEST_UNIT * 100)); - assert_eq!(triggered.get(), 0b0000); - assert_eq!(timer.poll(TEST_UNIT * 101).unwrap(), Sleep(TEST_UNIT * 10)); - assert_eq!(triggered.get(), 0b0001); -} - -#[test] -fn disabled_timers() { +fn disabled_timers_test() { let triggered = Cell::new(0); let mut timer = Xidlehook::new(vec![ @@ -89,12 +27,12 @@ fn disabled_timers() { triggered.set(0); // Make sure first timer is ignored - assert_eq!(timer.poll(TEST_UNIT * 00).unwrap(), Sleep(TEST_UNIT * 16)); + assert_eq!(timer.poll(TEST_UNIT * 00).unwrap(), Sleep(TEST_UNIT * 08)); assert_eq!(timer.poll(TEST_UNIT * 08).unwrap(), Sleep(TEST_UNIT * 08)); // ~timer 0~ assert_eq!(triggered.get(), 0b0000); assert_eq!(timer.poll(TEST_UNIT * 16).unwrap(), Sleep(TEST_UNIT * 04)); // timer 1 assert_eq!(timer.poll(TEST_UNIT * 20).unwrap(), Sleep(TEST_UNIT * 06)); // timer 2 - assert_eq!(timer.poll(TEST_UNIT * 26).unwrap(), Sleep(TEST_UNIT * 16)); // timer 3 + assert_eq!(timer.poll(TEST_UNIT * 26).unwrap(), Sleep(TEST_UNIT * 08)); // timer 3 assert_eq!(triggered.get(), 0b1110); // Now disable a timer in the middle and reset @@ -102,12 +40,12 @@ fn disabled_timers() { triggered.set(0); // Make sure first timer is ignored - assert_eq!(timer.poll(TEST_UNIT * 00).unwrap(), Sleep(TEST_UNIT * 16)); + assert_eq!(timer.poll(TEST_UNIT * 00).unwrap(), Sleep(TEST_UNIT * 08)); assert_eq!(timer.poll(TEST_UNIT * 08).unwrap(), Sleep(TEST_UNIT * 08)); // ~timer 0~ assert_eq!(triggered.get(), 0b0000); - assert_eq!(timer.poll(TEST_UNIT * 16).unwrap(), Sleep(TEST_UNIT * 06)); // timer 1 + assert_eq!(timer.poll(TEST_UNIT * 16).unwrap(), Sleep(TEST_UNIT * 04)); // timer 1 assert_eq!(timer.poll(TEST_UNIT * 20).unwrap(), Sleep(TEST_UNIT * 02)); // ~timer 2~ - assert_eq!(timer.poll(TEST_UNIT * 22).unwrap(), Sleep(TEST_UNIT * 16)); // timer 3 + assert_eq!(timer.poll(TEST_UNIT * 22).unwrap(), Sleep(TEST_UNIT * 08)); // timer 3 assert_eq!(triggered.get(), 0b1010); // Now disable all remaining timers and reset diff --git a/xidlehook-core/tests/first_timers.rs b/xidlehook-core/tests/first_timers.rs new file mode 100644 index 0000000..c2f755c --- /dev/null +++ b/xidlehook-core/tests/first_timers.rs @@ -0,0 +1,36 @@ +use std::{cell::Cell, time::Duration}; +use xidlehook_core::{timers::CallbackTimer, Action::*, Xidlehook}; + +const TEST_UNIT: Duration = Duration::from_millis(50); + +#[test] +fn first_timer_test() { + let triggered = Cell::new(0); + + let mut timer = Xidlehook::new(vec![ + CallbackTimer::new(TEST_UNIT * 100, || triggered.set(triggered.get() | 1)), + CallbackTimer::new(TEST_UNIT * 010, || triggered.set(triggered.get() | 1 << 1)), + CallbackTimer::new(TEST_UNIT * 050, || triggered.set(triggered.get() | 1 << 2)), + CallbackTimer::new(TEST_UNIT * 200, || triggered.set(triggered.get() | 1 << 3)), + ]); + + // Trigger all timers up to the last one. + assert_eq!(timer.poll(TEST_UNIT * 100).unwrap(), Sleep(TEST_UNIT * 010)); + assert_eq!(timer.poll(TEST_UNIT * 110).unwrap(), Sleep(TEST_UNIT * 050)); + + // The sleep would now be 200, except the first timer + // could be reactivated by activity so sleep is limited to 100. + assert_eq!(timer.poll(TEST_UNIT * 160).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(triggered.get(), 0b0111); + + timer.timers_mut().unwrap()[0].disabled = true; + + triggered.set(0); + + // Trigger all timers up to the last one. The sleep is always limited to 10 because the first + // enabled timer can be accessed at any time and thus 10 sleep is the minimum. + assert_eq!(timer.poll(TEST_UNIT * 010).unwrap(), Sleep(TEST_UNIT * 010)); + assert_eq!(timer.poll(TEST_UNIT * 060).unwrap(), Sleep(TEST_UNIT * 010)); + assert_eq!(timer.poll(TEST_UNIT * 260).unwrap(), Sleep(TEST_UNIT * 010)); + assert_eq!(triggered.get(), 0b1110); +} diff --git a/xidlehook-core/tests/general_timers.rs b/xidlehook-core/tests/general_timers.rs new file mode 100644 index 0000000..eee293d --- /dev/null +++ b/xidlehook-core/tests/general_timers.rs @@ -0,0 +1,66 @@ +use std::{cell::Cell, time::Duration}; +use xidlehook_core::{timers::CallbackTimer, Action::*, Xidlehook}; + +const TEST_UNIT: Duration = Duration::from_millis(50); + +#[test] +fn general_timer_test() { + let triggered = Cell::new(0); + + let mut timer = Xidlehook::new(vec![ + CallbackTimer::new(TEST_UNIT * 100, || triggered.set(triggered.get() | 1)), + CallbackTimer::new(TEST_UNIT * 010, || triggered.set(triggered.get() | 1 << 1)), + CallbackTimer::new(TEST_UNIT * 050, || triggered.set(triggered.get() | 1 << 2)), + CallbackTimer::new(TEST_UNIT * 200, || triggered.set(triggered.get() | 1 << 3)), + ]); + + // Test first timer + assert_eq!(timer.poll(TEST_UNIT * 0).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(timer.poll(TEST_UNIT * 20).unwrap(), Sleep(TEST_UNIT * 80)); + assert_eq!(timer.poll(TEST_UNIT * 40).unwrap(), Sleep(TEST_UNIT * 60)); + assert_eq!(timer.poll(TEST_UNIT * 74).unwrap(), Sleep(TEST_UNIT * 26)); + assert_eq!(timer.poll(TEST_UNIT * 99).unwrap(), Sleep(TEST_UNIT * 1)); + + // Trigger first timer + assert_eq!(triggered.get(), 0b0000); + assert_eq!(timer.poll(TEST_UNIT * 100).unwrap(), Sleep(TEST_UNIT * 10)); + assert_eq!(triggered.get(), 0b0001); + + // Test second timer + assert_eq!(timer.poll(TEST_UNIT * 103).unwrap(), Sleep(TEST_UNIT * 7)); + + // Overshoot second timer + assert_eq!(triggered.get(), 0b0001); + assert_eq!(timer.poll(TEST_UNIT * 500).unwrap(), Sleep(TEST_UNIT * 50)); + assert_eq!(triggered.get(), 0b0011); + + // Test third timer + assert_eq!(timer.poll(TEST_UNIT * 500).unwrap(), Sleep(TEST_UNIT * 50)); + assert_eq!(timer.poll(TEST_UNIT * 501).unwrap(), Sleep(TEST_UNIT * 49)); + assert_eq!(timer.poll(TEST_UNIT * 549).unwrap(), Sleep(TEST_UNIT * 1)); + + // Trigger third timer + assert_eq!(triggered.get(), 0b0011); + assert_eq!(timer.poll(TEST_UNIT * 550).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(triggered.get(), 0b0111); + + // Test fourth timer + triggered.set(0); + assert_eq!(timer.poll(TEST_UNIT * 600).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(timer.poll(TEST_UNIT * 649).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(timer.poll(TEST_UNIT * 650).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(triggered.get(), 0b0000); // no change + assert_eq!(timer.poll(TEST_UNIT * 680).unwrap(), Sleep(TEST_UNIT * 70)); + + // Trigger fourth timer + assert_eq!(triggered.get(), 0b0000); // no change + assert_eq!(timer.poll(TEST_UNIT * 750).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(triggered.get(), 0b1000); + + // It resets + triggered.set(0); + assert_eq!(timer.poll(TEST_UNIT * 0).unwrap(), Sleep(TEST_UNIT * 100)); + assert_eq!(triggered.get(), 0b0000); + assert_eq!(timer.poll(TEST_UNIT * 101).unwrap(), Sleep(TEST_UNIT * 10)); + assert_eq!(triggered.get(), 0b0001); +}