Skip to content

Commit

Permalink
When timers not stopping when stop() is called from activation callba…
Browse files Browse the repository at this point in the history 
…ck of an earlier timer

During timer activation, the active_timers list did not contain the future timer yet, so calling stop() would not succeed in removing from active_timers.

Fix this by making sure active_timers is up-to-date in terms of timers to be activated in the future by the time we start invoking the activation callbacks.
  • Loading branch information
@tronical
tronical committed Oct 14, 2024
1 parent 787e13b commit 906415b
Showing 1 changed file with 96 additions and 70 deletions.
166 changes: 96 additions & 70 deletions internal/core/timers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,100 +256,87 @@ impl TimerList {
CURRENT_TIMERS.with(|timers| {
assert!(timers.borrow().callback_active.is_none(), "Recursion in timer code");

let mut any_activated = false;

// The active timer list is cleared here and not-yet-fired ones are inserted below, in order to allow
// timer callbacks to register their own timers.
let timers_to_process = core::mem::take(&mut timers.borrow_mut().active_timers);
let expired_timers = {
let mut timers = timers.borrow_mut();
let expired_vs_remaining_timers_partition_point = timers
.active_timers
.partition_point(|active_timer| active_timer.timeout <= now);
let timers_not_activated_this_time =
timers.active_timers.split_off(expired_vs_remaining_timers_partition_point);
core::mem::replace(&mut timers.active_timers, timers_not_activated_this_time)
};

#[cfg(debug_assertions)]
{
let mut timer_ids = Vec::new();
for timer in &timers_to_process {
for timer in &expired_timers {
match timer_ids.binary_search(&timer.id) {
Ok(_) => assert!(false, "Duplicated active timer"),
Err(idx) => timer_ids.insert(idx, timer.id),
}
}
}

// Re-register all expired timers that are repeated, to ensure a consistent state when
// activating the callbacks later.
{
let mut timers = timers.borrow_mut();
for active_timer in &timers_to_process {
for active_timer in &expired_timers {
let timer = &mut timers.timers[active_timer.id];
assert!(!timer.being_activated);
timer.being_activated = true;

if matches!(timers.timers[active_timer.id].mode, TimerMode::Repeated) {
timers.activate_timer(active_timer.id);
} else {
timers.timers[active_timer.id].running = false;
}
}
}
for active_timer in timers_to_process.into_iter() {
if active_timer.timeout <= now {
any_activated = true;

let mut callback = {
let mut timers = timers.borrow_mut();

timers.callback_active = Some(active_timer.id);

// do it before invoking the callback, in case the callback wants to stop or adjust its own timer
if matches!(timers.timers[active_timer.id].mode, TimerMode::Repeated) {
// This timer might have been restarted from within an activation callback
// of an earlier timer, so don't register it twice.
if !timers.active_timers.iter().any(|t| t.id == active_timer.id) {
timers.activate_timer(active_timer.id);
}
} else {
timers.timers[active_timer.id].running = false;
}

// have to release the borrow on `timers` before invoking the callback,
// so here we temporarily move the callback out of its permanent place
core::mem::replace(
&mut timers.timers[active_timer.id].callback,
CallbackVariant::Empty,
)
};

match callback {
CallbackVariant::Empty => (),
CallbackVariant::MultiFire(ref mut cb) => cb(),
CallbackVariant::SingleShot(cb) => {
cb();
timers.borrow_mut().callback_active = None;
timers.borrow_mut().timers.remove(active_timer.id);
continue;
}
};

let mut timers = timers.borrow_mut();
let any_activated = !expired_timers.is_empty();

let callback_register = &mut timers.timers[active_timer.id].callback;
for active_timer in expired_timers.into_iter() {
let mut callback = {
let mut timers = timers.borrow_mut();

// only emplace back the callback if its permanent store is still Empty:
// if not, it means the invoked callback has restarted its own timer with a new callback
if matches!(callback_register, CallbackVariant::Empty) {
*callback_register = callback;
timers.callback_active = Some(active_timer.id);

// have to release the borrow on `timers` before invoking the callback,
// so here we temporarily move the callback out of its permanent place
core::mem::replace(
&mut timers.timers[active_timer.id].callback,
CallbackVariant::Empty,
)
};

match callback {
CallbackVariant::Empty => (),
CallbackVariant::MultiFire(ref mut cb) => cb(),
CallbackVariant::SingleShot(cb) => {
cb();
timers.borrow_mut().callback_active = None;
timers.borrow_mut().timers.remove(active_timer.id);
continue;
}
};

timers.callback_active = None;
let t = &mut timers.timers[active_timer.id];
if t.removed {
timers.timers.remove(active_timer.id);
} else {
t.being_activated = false;
}
let mut timers = timers.borrow_mut();

let callback_register = &mut timers.timers[active_timer.id].callback;

// only emplace back the callback if its permanent store is still Empty:
// if not, it means the invoked callback has restarted its own timer with a new callback
if matches!(callback_register, CallbackVariant::Empty) {
*callback_register = callback;
}

timers.callback_active = None;
let t = &mut timers.timers[active_timer.id];
if t.removed {
timers.timers.remove(active_timer.id);
} else {
let mut timers = timers.borrow_mut();
let t = &mut timers.timers[active_timer.id];
if t.removed {
timers.timers.remove(active_timer.id);
} else {
t.being_activated = false;
// This timer might have been restarted from within an activation callback
// of an earlier timer, so don't register it twice.
if !timers.active_timers.iter().any(|t| t.id == active_timer.id) {
timers.register_active_timer(active_timer);
}
}
t.being_activated = false;
}
}
any_activated
Expand Down Expand Up @@ -1115,3 +1102,42 @@ i_slint_core::tests::slint_mock_elapsed_time(120);
*/
#[cfg(doctest)]
const _RESTART_TIMER_BEING_ACTIVATED: () = ();

/**
* Test that a future timer can be stopped from the activation callback of an earlier timer.
```rust
i_slint_backend_testing::init_no_event_loop();
use slint::{Timer, TimerMode};
use std::{rc::Rc, cell::Cell, time::Duration};
let later_timer_expiration_count = Rc::new(Cell::new(0));
let sooner_timer = Timer::default();
let later_timer = Rc::new(Timer::default());
later_timer.start(TimerMode::SingleShot, Duration::from_millis(500), {
let later_timer_expiration_count = later_timer_expiration_count.clone();
move || {
later_timer_expiration_count.set(later_timer_expiration_count.get() + 1);
}
});
sooner_timer.start(TimerMode::SingleShot, Duration::from_millis(100), {
let later_timer = later_timer.clone();
let later_timer_expiration_count = later_timer_expiration_count.clone();
move || {
later_timer.stop();
}
});
assert_eq!(later_timer_expiration_count.get(), 0);
assert!(later_timer.running());
i_slint_core::tests::slint_mock_elapsed_time(110);
assert_eq!(later_timer_expiration_count.get(), 0);
assert!(!later_timer.running());
i_slint_core::tests::slint_mock_elapsed_time(800);
assert_eq!(later_timer_expiration_count.get(), 0);
assert!(!later_timer.running());
```
*/
#[cfg(doctest)]
const _STOP_FUTURE_TIMER_DURING_ACTIVATION_OF_EARLIER: () = ();

0 comments on commit 906415b

Please sign in to comment.